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

Prevent duplicative merge attempts. #2036

Merged

Conversation

rebeccacremona
Copy link
Contributor

@rebeccacremona rebeccacremona commented Jan 26, 2024

If you attempt to "publish" the same draft multiple times and make your requests quickly enough, multiple attempts may pass this test and proceed through the merging code.

That is never desirable.

And I have not yet definitively proved, but am quite convinced that this occasionally completely corrupts the casebook involved.

This PR uses select_for_update to ensure that only one publication attempt is ever in progress at once.

The user experience could still use some polishing: right now, on prod, if something goes wrong with the "publish" modal (for instance, you open two tabs with a draft, and click publish in one, wait a few seconds, and then click publish in the other), the modal just stays open. Your request gets a 400... but unless the dev tools are open, you don't see any notification.

I tried to fix that in this PR too, so that I could add an error message for THAT situation AND for the situation this PR fixes. But I couldn't figure it out...

But, I think this is worth it even without that: a weirdly not-closing modal is better than a potentially corrupted book!

@rebeccacremona rebeccacremona requested a review from a team as a code owner January 26, 2024 21:58
@rebeccacremona rebeccacremona requested review from teovin and bensteinberg and removed request for a team January 26, 2024 21:58
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (64981a3) 76.48% compared to head (6940e62) 76.55%.
Report is 1 commits behind head on develop.

Files Patch % Lines
web/main/models.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2036      +/-   ##
===========================================
+ Coverage    76.48%   76.55%   +0.06%     
===========================================
  Files           64       64              
  Lines         7071     7092      +21     
===========================================
+ Hits          5408     5429      +21     
  Misses        1663     1663              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rebeccacremona rebeccacremona removed the request for review from teovin January 26, 2024 22:13
@rebeccacremona rebeccacremona merged commit 8a58bb0 into harvard-lil:develop Jan 26, 2024
2 checks passed
Copy link

sentry-io bot commented Jan 29, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ValueError: This casebook's draft is already being merged. /casebooks/{casebook_param})/publish/ View Issue

Did you find this useful? React with a 👍 or 👎

@rebeccacremona rebeccacremona deleted the no-duplicative-merges branch March 11, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants