Skip to content

Fix attachment title migration generating possibly invalid values#8020

Merged
leio10 merged 1 commit intodecidim:developfrom
mainio:fix/attachment-title-migration
May 24, 2021
Merged

Fix attachment title migration generating possibly invalid values#8020
leio10 merged 1 commit intodecidim:developfrom
mainio:fix/attachment-title-migration

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented May 17, 2021

🎩 What? Why?

The FixAttachmentsTitles migration creates possibly duplicate title hashes.

The problem is that sometimes the title field is already a hash but the description field can still be a nil (i.e. not a hash).

This causes the following check in the migration to proceed with the migration:

next if attachment.title.is_a?(Hash) && attachment.description.is_a?(Hash)

This fixes the migration by ensuring that if the title or the description are already hashes, those columns won't be updated. If one of them is a hash and one of them is not, the migration will run for the column that does not have a hash.

📌 Related Issues

Testing

  • Create an attachment in the database with the title field being a translation hash and the description field being nil
  • Run this migration
  • See the record in the database after the migration

📋 Checklist

  • 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.

@leio10 leio10 added module: participatory processes type: fix PRs that implement a fix for a bug labels May 24, 2021
Copy link
Copy Markdown
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

Good job! 👍

@leio10 leio10 merged commit eedf7f4 into decidim:develop May 24, 2021
@ahukkanen ahukkanen deleted the fix/attachment-title-migration branch May 24, 2021 12:33
entantoencuanto added a commit that referenced this pull request May 31, 2021
* develop: (59 commits)
  Update supported versions in docs (#8079)
  Meetings merge minutes and close actions (#7968)
  Meeting calendars providers (#7944)
  Fix broken test on meetings after merging PR without rebase (#8076)
  Show participants list in meetings (#7933)
  Security feature external link warning (#7397)
  Add missing tests for scope types admin page (#8053)
  Use symbols for polymorphic route arguments (#8052)
  Mockup design for Participation statistics tables in Votings (#7879)
  Fix boolean fields for .reported? and .hidden? which is nil if no report exists (#7990)
  Fix redirects broken by Terms and Conditions redirect (#8036)
  Amend CSS overwritting (#8007)
  New Crowdin updates (#8048)
  Fix undetected broken tests because of missing dependencies (#8050)
  Validate results by Monitoring Committee Members (#7899)
  Electoral certificate validation by Monitoring Committee Members (#7871)
  Publish and unpublish a meeting (#7893)
  New Crowdin updates (#8005)
  Polling station closure attach the physical electoral closure certificate (#7929)
  Fix attachment title migration generating possibly invalid values (#8020)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: participatory processes type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants