Skip to content

Fix broken attachments with form validation errors#7336

Merged
mrcasals merged 11 commits intodecidim:developfrom
mainio:fix/broken-attachments-with-form-errors
Feb 11, 2021
Merged

Fix broken attachments with form validation errors#7336
mrcasals merged 11 commits intodecidim:developfrom
mainio:fix/broken-attachments-with-form-errors

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Feb 10, 2021 •
edited
Loading

🎩 What? Why?

When there is a form that has attachments on it (photos or documents), the form will break in case you submit it and the following is true:

  • You are updating an existing record
  • The existing record has attachments in it
  • You are NOT updating the attachments during the record update
  • You have updated some other field that causes a validation error on the form

Currently this can happen on the proposals form for instance (see testing).

The following kind of stack trace will appear in the console from this error:

ActionView::Template::Error (undefined method `thumbnail_url' for "179":String)
# ./app/views/decidim/proposals/proposals/_edit_form_fields.html.erb:70:in `block in __home_ahukkanen_dev_rails_decidim_decidim_proposals_app_views_decidim_proposals_proposals__edit_form_fields_html_erb__3656523159076971049_129540'
# ./app/views/decidim/proposals/proposals/_edit_form_fields.html.erb:68:in `each'
# ./app/views/decidim/proposals/proposals/_edit_form_fields.html.erb:68:in `__home_ahukkanen_dev_rails_decidim_decidim_proposals_app_views_decidim_proposals_proposals__edit_form_fields_html_erb__3656523159076971049_129540'
# ./app/views/decidim/proposals/proposals/edit.html.erb:20:in `block in __home_ahukkanen_dev_rails_decidim_decidim_proposals_app_views_decidim_proposals_proposals_edit_html_erb___252343347548760021_129320'
# /home/ahukkanen/dev/rails/decidim/decidim-core/app/helpers/decidim/decidim_form_helper.rb:34:in `decidim_form_for'
# ./app/views/decidim/proposals/proposals/edit.html.erb:19:in `__home_ahukkanen_dev_rails_decidim_decidim_proposals_app_views_decidim_proposals_proposals_edit_html_erb___252343347548760021_129320'
# ./app/controllers/decidim/proposals/proposals_controller.rb:186:in `block (2 levels) in update'
# ./app/commands/decidim/proposals/update_proposal.rb:30:in `call'
# ./app/controllers/decidim/proposals/proposals_controller.rb:178:in `update'
# /home/ahukkanen/dev/rails/decidim/decidim-core/app/controllers/concerns/decidim/use_organization_time_zone.rb:21:in `use_organization_time_zone'
# /home/ahukkanen/dev/rails/decidim/decidim-core/app/controllers/concerns/decidim/locale_switcher.rb:24:in `switch_locale'
# ./spec/controllers/decidim/proposals_controller_spec.rb:178:in `block (4 levels) in <module:Proposals>'
# ------------------
# --- Caused by: ---
# NoMethodError:
#   undefined method `thumbnail_url' for "179":String
#   ./app/views/decidim/proposals/proposals/_edit_form_fields.html.erb:70:in `block in __home_ahukkanen_dev_rails_decidim_decidim_proposals_app_views_decidim_proposals_proposals__edit_form_fields_html_erb__3656523159076971049_129540'

📌 Related Issues

Testing

  • Create a default Decidim development instance with seed data
  • Navigate to admin panel to one of the proposals components and enable attachments on it
  • Navigate to the front-end to create a new proposal
  • Create the proposal WITH ATTACHMENTS until the preview step
  • From the preview step, go back to modify the proposal
  • Modify the proposal title to a string length of 15 characters exactly (this is another bug that it causes validation error but allows replicating this issue)
  • See the error after submission

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

@ahukkanen
Fix issue when submitting records with attachments
cd327e4 
When submitting existing records that have attachments (photos and
documents) and there are validation errors on the form, the view
would break before because it assumed the submitted attachment IDs
are attachment records.
@ahukkanen
Add specs to test the broken attachment records case is covered
594b31a
@ahukkanen
Fix the example comment in the attachment attributes concern
d2acc93
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 10, 2021

@ahukkanen thank you so much for this PR!

I just merged #7054, which should solve the same issue but only for initiatives. Do you mind taking a look at it, see if there's any incompatibility with the two solutions? Git doesn't highlight any conflict, but both solutions are different so there might be some logic problem...

@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Feb 10, 2021

Thanks for the heads up @mrcasals !

I checked it and indeed, it fixes the same issue for the initiatives form.

I added a revert commit for #7054 to this PR and added that in the related PRs.

It wouldn't break even if it was there but #7054 is unnecessary once this is merged. There are also added tests in this PR that cover these cases to make sure.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 10, 2021

Perfect, @ahukkanen, thanks so much for taking care of this! ❤️

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 10, 2021

@ahukkanen some tests are failing! 👀 🔍

@ahukkanen
Add missing inclusions for the AttachmentAttributes concern
db88c0d
@ahukkanen
Fix gallery methods and multiple attachments methods
2bd4f6a 
After changing the forms to contain Attachment records, they were
not correctly handled in the commands as they added IDs to the
list of documents and photos.
@ahukkanen
Fix admin resource gallery examples after attachment changes
ab8003f
@ahukkanen
Rename "file" to "document" to better represent the variables
7250d76
@ahukkanen
Change the admin controller spec definition method
2248bc0 
In order to avoid collisions when running the specs in random
order, specify the controller to be tested specifically.
@ahukkanen
Move elections and proposals controller specs to correct paths
c23d752 
In order to fix Rubocop issues, move the controller specs to
correctly formatted directories.
@ahukkanen
Copy link
Copy Markdown
Contributor Author

ahukkanen commented Feb 11, 2021

@mrcasals Fixed the remaining issues and some failing specs.

Initiatives test set seems to pass for me locally and for consultations, the error is the same as in other PRs right now, so it is most likely unrelated to this PR. Maybe re-running would work (although the consultations test seems to fail constantly)?

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 11, 2021 •
edited
Loading

@ahukkanen I've restarted the tests, but I guess rebasing the PR from develop would solve it? I've merged the 2 PRs you created to solve those flakies!

@mrcasals mrcasals merged commit 1f3572b into decidim:develop Feb 11, 2021
@mrcasals mrcasals mentioned this pull request Feb 11, 2021
Merged
12 tasks
@ahukkanen ahukkanen deleted the fix/broken-attachments-with-form-errors branch February 11, 2021 12:23
@oliverbarnes oliverbarnes mentioned this pull request Mar 16, 2021
Closed
This was referenced Mar 26, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

module: budgets module: core module: elections module: forms module: initiatives module: proposals type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahukkanen @mrcasals