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

Fix attachment title migration generating possibly invalid values #8020

Merged
merged 1 commit into from
May 24, 2021

Conversation

ahukkanen
Copy link
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
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