-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Fix edge case on saving profile attachments #4472
Comments
Hi, is it all right if I start working on this issue? |
Yes, please. |
Hi @cielf and team, could I please get some early feedback on my approach to fixing this bug? My plan was to prevent the user from being able to persist invalid changes to the profile. If they attach a document, change the client share to an invalid number, and then click update, they will see an alert at the top of the page. Also, the document won't be attached, and the changes to the client share won't be persisted. This definitely breaks tests. Here is a compare link, so that you can see the very beginning of what I am suggesting. What are your thoughts on this approach? Do you support it, or should I take a different tack? Thanks! |
@McEileen I don't think this is the best approach. If we redirect when the update fails, then we lose all the changes the user made in the form. The partner profile has lots of fields. So it would be possible for the user to fill in a bunch of fields, click save, and then lose all of the new information they put in due to a validation error. Which could be pretty frustrating for users. Have you seen this open Rail issue? Seems like people solve this issue in one of two ways: ActiveStorage direct uploads or adding a guard clause to make sure attachments have been persisted. Not sure which option the maintainers would prefer. But I think the second option (along with adding a flash message when an attachment failed to be persisted) is the one I would lean towards. |
@McEileen --- I am now wondering if work on this this issue might be in conflict with #4821 -- a rework of the profile attachments that @danielabar is working on. Certainly, @danielabar needs to know about this. As for the technical aspects - I'm afraid I spend very little of my time with the code, so I'm going to ask @awwaiid or @dorner to chime in. |
I was reading a little more about direct uploads here and it mentions:
So maybe it is worth implementing direct uploads? Only drawback I see is that it would require some CORS configuration in production. What do you think @dorner? |
I'm glad I asked for feedback! @coalest, I hadn't seen that open Rails issue. Thanks for sharing it, it's very helpful. I'm curious what @dorner and @awwaiid think, too. @cielf Thanks for flagging that work on this issue might be in conflict with #4821. Should I hold off on working on this issue until @danielabar is done with #4821? If so, I am happy to pick up another issue. Just let me know, thanks! |
Now I realize I was thinking this had to do with the "Additional Documents" upload, but it has to do with the IRS Determination Letter. I therefore think it probably doesn't conflict. So do please keep working on this one, if you want to. |
For the other issue #4821 , the bug occurs if a user saves one document, then comes back later to add another one, the first one disappears. But from the discussion on this ticket, it looks like there could be an unexpected interaction with document uploads and other form validation errors so I'll keep that in mind. |
There is a potentially simpler solution than direct uploads and CORS, which would prevent the 500 server error and let the user know they need to select the document again. See this comment and commits. |
@cielf should we combine these two issues? I feel like whatever solution we have probably should work for both of them. I agree that ideally we should be able to keep track of files that were uploaded even when there are form errors and not ask the user to re-select them. @danielabar were you able to figure this out without direct upload? |
@dorner My solution is an "in between" - I can detect that there was a new doc attempt, but it all gets rolled back with the validation error. So my solution will inform the user via an alert that they need to fix the validation error(s), then re-attach the doc(s). It would look like this: |
@dorner As I wrote the above comment, just realized - given that it's possible to detect a new doc is being uploaded, it may also be possible to save, just that portion of the profile, even if there are validation errors in other fields. I'll investigate further. |
@McEileen -- If you haven't made real progress on this, it looks like there might be, not a conflict with, but synergy with just combining it with #4821. So it probably makes sense to have @danielabar work on both after all. |
@cielf That makes sense. I will remove myself from this issue and @danielabar can be assigned to it. I'll look into picking up another issue. |
Sorry for long post, there's a few angles to consider. Here is some analysis on the problem and potential solutions. This is reported specifically here but also affects #4821. More broadly, the problem occurs in any edit form in Rails that:
If a user uploads a new attachment(s) and fills out other form fields that result in a validation error, the following problems occur: When the edit form re-renders, it attempts to display the attached file name(s). Since the user attempted an upload, there is an association to a document, however, it's not persisted. This results in the Another problem is that the file doesn't get uploaded and associated with the model, because this depends on the model being saved successfully. So the user would have to realize this and attempt their upload again. But they're currently on a 500 Error Page and are completely stuck. This behaviour is described in the Rails Guide on Active Storage in the section on Form Validation. Options1. Avoid ErrorThe simplest solution to avoid the 500 error is to update the edit view to check if a document is persisted? before attempting to render it. For Partner Profile <% profile.documents.each do |doc| %>
<% if doc.persisted? %>
<li><%= link_to doc.blob['filename'], rails_blob_path(doc), class: "font-weight-bold" %></li>
<% end %>
<% end %> Note that some of the existing Partner Profile edit view partials are checking for attached?, but this returns true when the document is not persisted so it's insufficient as a safety check to avoid errors. Pros
Cons
2. Avoid Error + DetectionMake the same view changes as in solution 1 above to avoid the error page. PLUS Change the controller Specifically for partner profile attached documents association, the updated controller action is: module Partners
class ProfilesController < BaseController
def update
# === CHECK IF USER IS ATTEMPTING AN UPLOAD ===
new_document_uploaded = detect_new_documents?(profile_params)
result = PartnerProfileUpdateService.new(current_partner, partner_params, profile_params).call
if result.success?
# Handle success...
else
flash.now[:error] = "There is a problem. Try again: %s" % result.error
# === ALERT IN CASE OF VALIDATION ERROR AND UPLOAD ATTEMPT ===
flash.now[:alert] = "The file you uploaded was not saved due to validation errors. Please reattach it after fixing the errors." if new_document_uploaded
# ...
end
end
private
# === NEW METHOD TO CHECK IF USER WAS TRYING TO UPLOAD DOCS ===
def detect_new_documents?(profile_params)
profile_params[:documents].any? { |doc| doc.is_a?(ActionDispatch::Http::UploadedFile) }
end
end
end Pros
Cons
3. Avoid Error + Detect + Force SaveMake the same view changes as in solution 1 above to avoid the error page. PLUS Change controller Here is some very experimental code but it has problems: module Partners
class ProfilesController < BaseController
def update
# === CHECK IF USER IS ATTEMPTING AN UPLOAD ===
new_document_uploaded = detect_new_documents?(profile_params)
result = PartnerProfileUpdateService.new(current_partner, partner_params, profile_params).call
if result.success?
# Handle success...
else
flash.now[:error] = "There is a problem. Try again: %s" % result.error
# === TRY TO FORCEFULLY SAVE ===
save_documents(profile_params) if new_document_uploaded
# ...
end
end
private
# === NEW METHOD TO CHECK IF USER WAS TRYING TO UPLOAD DOCS ===
def detect_new_documents?(profile_params)
profile_params[:documents].any? { |doc| doc.is_a?(ActionDispatch::Http::UploadedFile) }
end
# === TRY TO FORCEFULLY SAVE DOC(S) ===
def save_documents(profile_params)
# We have to load the last saved (i.e. valid) profile from the database
# to perform a save operation because anything based on `profile_params`
# will contain validation errors.
current_partner_profile_id = current_partner.profile.id
temp_profile = Partners::Profile.find(current_partner_profile_id)
# Grab the documents user was attempting to upload and save them
documents = profile_params[:documents]
temp_profile.documents.attach(documents)
temp_profile.save!
# Now we have a problem - if we simply return here, then the `current_partner.profile` model
# will not display the attachment because it still has the old state.
# So we can reload it so it reflects the fresh state from the database
# BUT lose the invalid data which user should see to fix other validation errors
current_partner.profile.reload
end
end
end Pros
Cons
4. Direct UploadsUPDATE 2024-12-20: After doing some experimenting, it turns out Direct Upload will not help with associating a file with a model in the case of validation errors.. All of the above solutions involve server side processing. With Direct Uploads, it uses client side JavaScript to directly upload the file from the browser to the server. Pros
Cons
SuggestionMy inclination is for Option 2 (Avoid Error + Detection) as it resolves both issues in a relatively simple way. Pending user feedback (for example - how often does it occur that user was uploading a doc and also encountered validation error), could add Option 4 as a future enhancement. |
I think we can safely say #1 and #3 have too many downsides to consider them. #4 being "the Rails way" points me more towards it. There are things built into the framework designed to make this work without much effort. I'm not too fussed about the CORS setup, which is something that shouldn't take long to do. The additional JS is just a couple of lines, so shouldn't matter much. In general, if there's a general-purpose solution that's used by lots of other projects, I'd much prefer doing it that way. #2 is less work but it results in a less than ideal user experience. The nice thing about this is that once it's done, we can put it in the application JS and it would work for any other documents we'd ever add. The cron job to purge attachments isn't urgent and can be a followup (we might not really need to do it at all - we probably won't see more than a handful of these every month/year/quarter). |
Also noting that we do have another document upload that this will apply to -- one on the partner itself which is meant for documents the bank sees about the partner, that the partner doesn't. I'm pretty sure that'll be the last one. There's a proto-issue in the hopper for that. |
Posting some early findings: Here's what happens when direct upload is enabled, and user submits a form with a new file attachment and form inputs that are invalid: As soon as form submitted, direct upload begins from the browser to the storage service. And this completes successfully. Locally this results in a new file being persisted in the storage service. For example, output of my
It also creates a new record in database table
However, there is no record in
In the Rails server output, it starts processing the form update:
So even though the file has been uploaded successfully, Rails still won't associate the model with it due to the validation error. And this leaves an orphan file in storage, which must be why the Rails Guides mention to have a cron job to periodically clean up. Still Hope?I found this blog post: https://justin.searls.co/posts/drive-by-active-storage-advice/
I can attempt the hack mentioned in the blog post, but it would appear that there is no entirely Rails-way to solve this. This is also the kind of thing to keep an eye on if it would still work on future Rails upgrades. |
Related to above, just found this mention of a hidden field together with direct uploads in Rails Guide on ActiveStorage so maybe it's not a hack after all? https://guides.rubyonrails.org/v7.1/active_storage_overview.html#form-validation Will investigate further. |
Doing some more experimenting with direct uploads, and hidden field to maintain the upload in case of form validation errors: NOTE: direct upload only starts when user submits the form. When the form renders with validation error, the blob has been uploaded to file storage and saved in the database. And the model This means it can be rendered something like this: <label class="control-label col-md-3">501(c)(3) IRS Determination Letter</label>
=== EXISTING SUCCESS CASE WHEN FILE IS FULLY ATTACHED AND PERSISTED ===
<% if profile.proof_of_partner_status.persisted? %>
<div class="col-md-8">
Attached file: <%= link_to profile.proof_of_partner_status.blob['filename'], rails_blob_path(profile.proof_of_partner_status), class: "font-weight-bold" %>
<%= pf.file_field :proof_of_partner_status, direct_upload: true, class: "form-control-file" %>
</div>
<% else %>
<div class="col-md-8">
=== NEW LOGIC: SUBMIT HIDDEN FIELD WITH SIGNED_ID BECAUSE IT IS THERE IN MEMORY ===
=== ALSO SHOW USER A "DON'T WORRY" MESSAGE WITH THE FILE NAME ===
<% if profile.proof_of_partner_status.attached? %>
<%= pf.hidden_field :proof_of_partner_status, value: profile.proof_of_partner_status.signed_id %>
Don't worry we haven't lost your upload of <span class="font-mono"><%= profile.proof_of_partner_status.blob.filename %></span>, you don't need to to upload it again
<% end %>
<div class="col-md-8">
<%= pf.file_field :proof_of_partner_status, direct_upload: true, class: "form-control-file" %>
</div>
<% end %> This works for the first time user is uploading the IRS letter, or if they already had one saved, and want to switch to another one. Since there are multiple of these files, I could look into extracting that logic to a partial. The has_many_attached for other documents requires further research because that also requires hidden fields just to maintain the existing ones if user is uploading one more. NOTE: They still have to fix the validation errors and submit the form again successfully, in order for the file attachment to actually get associated with the model. i.e. if they abandon the form at this point, the file will not be associated with the profile. @cielf Does this sound like a reasonable interaction to solve this issue? |
I'm not immediately getting why we can't show them the information where they would expect it. |
The interaction would look like this:
All of this is committed on my branch if you want to try it out: https://github.com/danielabar/human-essentials/tree/4472-partner-profile-files-direct-upload |
I'm not clear on why we can't show the name of the file in the intervening bit. |
Hrm - I tried it out and it wasn't preserving the file information if you change the attached file, then have an error, then fix the error. |
Sorry for taking so long to get to this. I think we might be barking up the wrong tree a bit (and as usual, it's on me). The simplest thing to do might be to just say "Attached file:", instead of all the "Don't worry..." which I think might actually make the partners worry more when they weren't worrying before? Yes -- technically it's not actually attached yet. But it will be if they successfully save, and none of the other information they have entered will be saved unless they save successfully, so it is still consistent. |
In the whole keeping things consistent vein, though -- please also look at how we are handling the file uploads on the partner (as opposed to the profile). This is just as input into your work -- In an ideal world, we should be doing the same sort of thing across the board. |
And yeah -- what you have now does seem to handling showing the file with error properly. |
@cielf I've removed the "don't worry" message. I'm currently work-in-progress on a re-usable solution for the custom file input. Features will include:
This could eventually be used to replace all file inputs, but might want to limit this to Partner Profile to limit the scope. The work here will include:
NOTE: This will handle single file attachments. The multiple case has other complexities, there's a different issue to deal with that: #4821 |
Yes, Step wise has been turned on in prod. And yes, we will be getting rid of the old legacy form as a follow-on. I'll talk with the planning group about timing on that tomorrow. |
Ensure Active Storage direct uploads retain selected files during form resubmissions when validation errors occur. This improves the user experience by avoiding the need to re-select files. - Add a custom file input partial styled with Bootstrap to display file names for new and previously uploaded files. - Include a hidden field with the signed ID of uploaded files for re-association after successful submission. - Create a Stimulus controller (`file-input-label`) to dynamically update labels with the selected file name or default text. - Update forms in `Partners::Profiles` to use the shared file upload partial. - Add a system test to validate the persistence of file uploads through validation errors and final submission.
Static error pages were previously loading the entire app's JavaScript, including components like ActiveStorage, which aren't needed on these pages. This was causing JavaScript console errors in system tests due to missing importmap references for ActiveStorage. Instead of adding unnecessary imports, this commit removes the JavaScript from the static error pages.
Summary
There is a sequence on editing a profile with an upload that results in the file not being saved
Why fix?
Not saving things the users reasonably expect to be saved is not good... and we have seen the exception noted below in production.
Details
Recreation
Try this sequence:
Sign in as [email protected]
Edit My Organization
Choose a file for IRS Determination Letter
Change the numbers in Area Served so that it no longer adds up to 100
Click Update Information
gets an exception "Cannot get a signed_id for a new record"
What we need
That sequence should not produce an exception. If it is not going to result (after correcting other errors) in the file chosen being saved, we need to include that in the error messaging
Criteria for completion
The text was updated successfully, but these errors were encountered: