Skip to content
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

feat: add alert when missing attachment #1220 #1273

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mgrigoriev8109
Copy link
Contributor

🔗 Issue

#1221

✍️ Description

Adds an alert when a staff user attempts to upload 0 files or images

📷 Screenshots/Demos

bin/dev, log in as Staff, head to http://localhost:3000/alta/staff/pets/2?active_tab=photos

Click Attach without any photo uploaded, and see the new alert.
Screenshot 2024-12-17 at 3 04 09 PM

Add an image and check that the success alert pops up as it was intended.
Screenshot 2024-12-17 at 3 04 35 PM

Do the same for Files
Screenshot 2024-12-17 at 3 05 06 PM
Screenshot 2024-12-17 at 3 05 24 PM

@mgrigoriev8109
Copy link
Contributor Author

Need to tinker with this a bit more, should've ran the tests locally - looks like having my method as a before_action for those actions is breaking some stuff.

Copy link
Collaborator

@kasugaijin kasugaijin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a couple small comments.
Run rails standard:fix to fix the Standard pipeline failure

@mgrigoriev8109
Copy link
Contributor Author

@kasugaijin this is ready for a re-review. The :images and :files params behaved differently, just like you predicted.

In a live environment for some reason they're populated with an empty string, so they become an array of ["", {image-object}, {image-object}].

I figured instead of adding empty strings to the test environment it'd be cleaner to just remove those empty strings in the method. Let me know if this works, or if I should implement it differently.

render turbo_stream: turbo_stream.replace("flash", partial: "layouts/shared/flash_messages")
end

def show_alert_if_attachment_missing
if !params[:pet][:images].nil?
Copy link
Collaborator

@kasugaijin kasugaijin Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more idiomatic to use
if params[:pet][:images].present? rather than a double negative

same for the other condition for files


def show_alert_if_attachment_missing
if !params[:pet][:images].nil?
no_empty_images = params[:pet][:images].reject { |image| image == "" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no_empty_images could just be renamed images, same for files.

if !params[:pet][:images].nil?
no_empty_images = params[:pet][:images].reject { |image| image == "" }
elsif !params[:pet][:files].nil?
no_empty_files = params[:pet][:files].reject { |image| image == "" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add a comment on why we are rejecting the ""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants