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 duplicated endorsements #11853

Merged
merged 7 commits into from Nov 9, 2023
Merged

Fix duplicated endorsements #11853

merged 7 commits into from Nov 9, 2023

Conversation

alecslupu
Copy link
Contributor

@alecslupu alecslupu commented Oct 27, 2023

🎩 What? Why?

This PR tries to fix a concurrency issue that happens in endorsements. As per PostgreSQL documentation, null values in a unique column are not considered equal, allowing multiple nulls in the column.

Testing

  1. Try to do multiple endorsements in the same proposal with the same user. See that you can't do that
  2. Before patching, make the multiple endorsements. Then run bundle exec rails decidim:upgrade:fix_duplicate_endorsements

♥️ Thank you!

@alecslupu alecslupu added the type: fix PRs that implement a fix for a bug label Oct 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 29, 2023
@alecslupu alecslupu marked this pull request as ready for review October 30, 2023 05:42
@alecslupu alecslupu requested a review from a team October 30, 2023 05:42
@alecslupu alecslupu changed the title Add Migration to set user_group_id default value to 0 Add migration to set user_group_id default value to 0 Oct 30, 2023
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I tried it locally and it seems like it fixed this problem.

I think we should add a rake task with a comment in the RELEASES_NOTES to fix the (possible) cases where there's more than one endorsement. What do you think?

@andreslucena andreslucena self-assigned this Oct 30, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 30, 2023
@andreslucena andreslucena changed the title Add migration to set user_group_id default value to 0 Fix duplicated endorsements Nov 1, 2023
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

It's preventing the issue with the more than one endorsement with the same user in the same resource, so on that side 🆗

Regarding the rake task, I think it's messing with the functionality, as it isn't allowing me to "Unlike" after I've liked:

Screenshot showing the like by the user

Screenshot showing that it isn't liked (?)

And when I click in my username in the modal, I see that the server is responding with 422 errors.

What I would expect is to see the "green"/selected identity in that modal, and then to be able to "unlike" it (and like it again).

Can you check that out please?

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@alecslupu
Copy link
Contributor Author

It's preventing the issue with the more than one endorsement with the same user in the same resource, so on that side 🆗

Regarding the rake task, I think it's messing with the functionality, as it isn't allowing me to "Unlike" after I've liked:

Screenshot showing the like by the user

Screenshot showing that it isn't liked (?)

And when I click in my username in the modal, I see that the server is responding with 422 errors.

What I would expect is to see the "green"/selected identity in that modal, and then to be able to "unlike" it (and like it again).

Can you check that out please?

@andreslucena I have checked this, and I could not find anything wrong with it. There caching issue.
The scenario I have tested is the following:

  • Visited a Proposal
  • Endorsed
  • Applied the patch
  • Refreshed the page ( for some reason, in the first few refreshes I was being listed in endorsers list, yet, i could press Like)
  • After i got enough refreshes ... eventually i have been able to see the Dislike button ..

Can you check that is not the same case on your end?
Also, could you also make sure you restart the server before actually trying to like / dislike ?

@andreslucena
Copy link
Member

There caching issue.

Bitten by the cache, once again!

Meme of "I don't always clean my cache... but when I do, it works"

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@andreslucena
Copy link
Member

Only failing check is Codecov as usual, so I'm merging this

@andreslucena andreslucena merged commit 7b840d2 into develop Nov 9, 2023
105 of 106 checks passed
@andreslucena andreslucena deleted the fix/endorsements branch November 9, 2023 12:51
@alecslupu
Copy link
Contributor Author

There caching issue.

Bitten by the cache, once again!

Meme of "I don't always clean my cache... but when I do, it works"

And I got bitten by proper English grammar :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix PRs that implement a fix for a bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants