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

Missing conflict handling when editing / adding / removing contributors #1568

Closed
netsensei opened this issue May 22, 2024 · 4 comments · Fixed by #1700
Closed

Missing conflict handling when editing / adding / removing contributors #1568

netsensei opened this issue May 22, 2024 · 4 comments · Fixed by #1700
Assignees
Labels
bug Something isn't working cypress-test-requested

Comments

@netsensei
Copy link
Contributor

Bug description

When I add, remove or order a contributor from a publication or dataset which was being edited by someone else; I don't see a warning that a conflict error has happened. However, when I reload the page, I notice that my change weren't saved: I see the changes made by the other person who was editing instead.

Steps to Reproduce

  1. Open the detail page of a publication & go to the "people & affiliations" tab (A)
  2. Add three new authors.
  3. Duplicate the page in a separate browser tab (B)
  4. Remove the authors in B.
  5. Go back to A and reorder the 3 authors you're seeing.
  6. Now add an author in A.
  7. Upon hitting the "save" button in the suggest box; notice how the table refreshes however your changes to the ordering are suddenly gone along with the authors who were just there a minute ago.

Automatic testing scenario

Write a high-level way to test this with Cypress if applicable.

Risk classification

Likelihood

Probable

Consequences

Critical. If multiple people add, remove & reorder contributors at the same time, they'll see contributors inexplicably appear and disappear because there's no indication someone else is working on the same record at the same time.

Expected behavior

All mutative actions should give you a conflict error dialog whenever someone else made a change.

@netsensei netsensei added the bug Something isn't working label May 22, 2024
@netsensei netsensei moved this to To refine 🧚 in Biblio overview Jun 11, 2024
@nicolasfranck
Copy link
Contributor

nicolasfranck commented Jun 13, 2024

What I noticed in the logs is that NOT conflict resolution is hit, but far before that, the code that checks if the length of the supplied position equals the length of the current contributors:

That is also a kind of conflict resolution, not? Any way that fails silently indeed.

Possible solution could be to compare the supplied _version against the stored publication/dataset.

Or remove the check on length altogether, and let the version-conflict handle the rest?
If the length is not equal to length in the database, then there is probably a different version, not?
The table of contributors is always reloaded for every action (edit/delete/order) any way.

@netsensei
Copy link
Contributor Author

Or remove the check on length altogether, and let the version-conflict handle the rest?

Changing the order should create a new snapshot each time. So, this should be treated as a version conflict. I don't think the extra check on length adds value here.

Make sure cypress tests are adjusted accordingly to catch the above case as well.

@verheyenkoen
Copy link
Contributor

verheyenkoen commented Sep 6, 2024

Or remove the check on length altogether, and let the version-conflict handle the rest?

Changing the order should create a new snapshot each time. So, this should be treated as a version conflict. I don't think the extra check on length adds value here.

The extra check for length did have it's purpose, since index-based copying over to a new slice without precaution would result in index-out-of-array-bounds errors. I improved the copying to make it safe and removed the check. Now it does display the concurrency error (+ small extra HTMX swapping fix for dataset creators).

@verheyenkoen
Copy link
Contributor

Found another somewhat related bug, described in #1701.

verheyenkoen added a commit that referenced this issue Sep 9, 2024
nics pushed a commit that referenced this issue Sep 11, 2024
* Fixed reorder contributors concurrency bug #1568

* Added concurrency tests for all contributor types when reordered in another session

* Cypress: hooked up drag command the same way as other commands

* Improved test structure #1568

* Fixed test comments

* Using cy.verifyConflictErrorDialog() command
@nics nics closed this as completed Sep 11, 2024
@github-project-automation github-project-automation bot moved this from To refine 🧚 to 🧪 To test in Biblio overview Sep 11, 2024
@mietcls mietcls moved this from 🧪 To test to Done 🪅 in Biblio overview Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cypress-test-requested
Projects
Status: Done 🪅
Development

Successfully merging a pull request may close this issue.

4 participants