-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lydia/wri 13 image upload #25
base: remove-editor.js
Are you sure you want to change the base?
Conversation
WRI-13 Image upload
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested.
- Reviewed the entire pull request up to 01a1a96
- Looked at
464
lines of code in12
files - Took 2 minutes and 21 seconds to review
More info
- Skipped
39
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. /db/migrate/20230317132606_create_active_storage_tables.active_storage.rb:2
:
- Assessed confidence :
50%
- Comment:
This migration creates new tables for ActiveStorage. Ensure that this migration is run carefully to avoid any conflicts or issues. - Reasoning:
The PR author has added a new migration to create ActiveStorage tables. This is necessary for using ActiveStorage. However, it's important to note that running this migration will create new tables in the database, so it should be done carefully to avoid any conflicts or issues.
Workflow ID: wflow_x4l3blcV14ZiGOXo
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
if current_user.nil? || @post.user != current_user | ||
ahoy.track "post viewed", post_id: @post.id | ||
end | ||
ahoy.track "post viewed", post_id: @post.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ahoy.track
method is called unconditionally, which means it will track views even for the post's author. This could lead to inaccurate tracking data. Consider restoring the condition to exclude the post's author from tracking.
@@ -66,7 +64,7 @@ def update | |||
respond_to do |format| | |||
if @post.update(post_params) | |||
format.turbo_stream {} | |||
format.html {redirect_to post_path(@post)} | |||
format.html {redirect_to root_path} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redirect path after updating a post has been changed to the root path. Consider changing it back to post_path(@post)
to redirect users to the updated post's show page.
@@ -53,7 +53,7 @@ | |||
config.action_mailer.asset_host = 'http://localhost:3000' | |||
|
|||
# Store uploaded files on the local file system (see config/storage.yml for options). | |||
config.active_storage.service = :local | |||
config.active_storage.service = :amazon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ActiveStorage service has been changed to :amazon in the development environment. Consider changing it back to :local to avoid unnecessary costs and slower upload/download speeds associated with cloud storage in the development environment.
@@ -134,7 +154,7 @@ def pending_acceptance? | |||
end | |||
|
|||
def visited_posts | |||
post_ids = Ahoy::Event.where(user_id: id , name: "post viewed").order(time: :desc).pluck(Arel.sql("properties ->> 'post_id'")) | |||
post_ids = Ahoy::Event.where(user_id: 2 , name: "post viewed").order(time: :desc).pluck(Arel.sql("properties ->> 'post_id'")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user_id for the Ahoy::Event.where query is hardcoded to 2. Consider replacing it with the id of the current user.
Summary:
This PR introduces image upload functionality, modifies application styles, and changes the active storage service to Amazon S3.
Key points:
PostsController
to handle image uploadsPost
andUser
models to handle image attachmentsapplication.js
andeditor_controller.js
to handle image uploads on the client sideapplication.tailwind.css
to update application stylesdevelopment.rb
andstorage.yml
Generated with ❤️ by ellipsis.dev