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

Enable concurrent update for Entities #843

Merged

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented Aug 16, 2023

image

What has been done to verify that this works as intended?

Automated tests and manually testes

Why is this the best possible solution? Were any other approaches considered?

NA

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

None

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

NA

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@sadiqkhoja sadiqkhoja requested a review from ktuite August 16, 2023 21:00
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Just a one-off thought, happy to also take a closer look if @ktuite prefers.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to change afterUpdate() as well so that the OData is updated with the correct version after the update. Right now, I think it wouldn't be possible to update the same entity twice in a row.

Copy link
Member

Choose a reason for hiding this comment

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

tried this (with sadiq's recent commit that makes this fix) and it works to update an entity multiple times

@matthew-white
Copy link
Member

There's a parallel change to Backend (getodk/central-backend#832), but the Backend issue is more about the API and database and won't need testing by the QA team. I think I'll slot this PR in the project separately, then add the "needs testing" label to just this PR. Then the QA team can focus on verifying the changes here.

Copy link
Member

@ktuite ktuite left a comment

Choose a reason for hiding this comment

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

My only nitpick (and I'm not sure what to suggest instead) is with the styling around the conflict alert: there's a little gap above the top red border, and the X to close the alert doesn't have a useful function there's nothing to "retry" there.

Screen Shot 2023-08-25 at 4 13 04 PM

Here's the same alert in a different modal with more padding.

Screen Shot 2023-08-25 at 4 17 24 PM

Copy link
Member

Choose a reason for hiding this comment

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

tried this (with sadiq's recent commit that makes this fix) and it works to update an entity multiple times

@sadiqkhoja
Copy link
Contributor Author

@getodk/testers this feature can be tested on staging

How to test

  • Open entities data page on two browser tabs / windows
  • Modify a row on one tab
  • Try to modify same row on another tab, you should see error message

@srujner
Copy link

srujner commented Sep 4, 2023

@sadiqkhoja
I found one thing that might be an issue.

Scenario:
User A opens Edit from Data tab first and starts editing the Entity.
User B opens Edit from Data tab second and is unable to edit the Entity.

Now as User B I'm able to close the modal -> click on More in the "Actions" tab -> Click on Edit next to Entity Data -> Edit any information in the Entity and Update it while User A is still editing the same Entity.

After that User A is unable to finish editing the Entity.

Maybe it'll be better to block editing from both tabs? Because right now it's easy to take over.

@lognaturel
Copy link
Member

@srujner, I think your comment is somewhat related to getodk/central#476. Ideally clicking on "Edit" would immediately check the entity version. It wouldn't help with a case in which two people/tabs already had the edit modal open but it would help in the much more common case where two people have the entity list or entity detail page open at the same time.

@srujner
Copy link

srujner commented Sep 5, 2023

Tested with Success!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

5 participants