Fix #19644: Remove user role from UI, redisplay list of users in the event of a backend failure

raafeahmed commented 3 weeks ago

Overview

This PR fixes #19644.
This PR does the following: • Modifies the removeRoleAsync function by first making a copy of the list of users prior to the deletion. This is for in the event of an error from the backend or if the deletion was unsuccessful. Next, the user role is immediately removed from the frontend. This is to prevent the user from attempting to delete the user role twice in the event of a delay from the server. Finally, a catch block is added to account for an unsuccessful deletion or error from the backend. This is for reverting/ redisplaying the list of users to the state prior to the deletion attempt using the copy made earlier, and alerts the end user that the user could not be removed. • Tests this functionality. The first test checks whether the user was removed immediately from the UI while the backend continues to process the deletion request. Once the backend returns, the frontend becomes in sync with the backend data. The second test redisplays the list of users by forcing an error to be thrown. It verifies that the user is initially removed from the UI before the backend returns, and then the UI reverts back to its previous state once the backend returns an error. Both tests are passing.
The original bug occurred because there's a server delay when deleting users through the Settings tab in the exploration editor. The user is not removed until the deletion has fully completed, so the same user may be deleted twice and this causes the error to occur. By immediately removing the user from the frontend while the backend processes the actual deletion request, the end user will never have the ability to delete a role twice. There is no associated PR that introduced this bug, but rather just the cause of server delays that happen periodically.

Essential Checklist

Please follow the instructions for making a code change.

[x] I have linked the issue that this PR fixes in the "Development" section of the sidebar.
[x] I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
[x] I have written tests for my code.
[x] The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
[x] I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Before:

https://github.com/oppia/oppia/assets/123130903/276c8fed-9b3f-46f9-856f-bc340d45633b The second attempt is what raises the error linked in the issue.

EDIT: updated screen recordings (5/2)

After:

https://github.com/oppia/oppia/assets/123130903/56958ff3-bdc5-449c-a256-b5c9a97542da This is again shown with a slight delay, time.sleep(2), to demonstrate that the user is removed immediately with the new code. Then once the backend successfully deletes the user, the confirmation is displayed to the end user. In practice, without the extra delay, the confirmation appears almost immediately.

https://github.com/oppia/oppia/assets/123130903/683b4690-2786-4ff3-8719-7801aa04c9fb Performs the same with the new code by keeping the user list exactly the way it was before the deletion was attempted and alerts the user.

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

oppiabot[bot] commented 3 weeks ago

Assigning @Lawful2002 for the first pass review of this PR. Thanks!

oppiabot[bot] commented 1 week ago

Hi @raafeahmed, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

oppiabot[bot] commented 1 hour ago

Unassigning @Lawful2002 since they have already approved the PR.

oppiabot[bot] commented 1 hour ago

Hi @raafeahmed, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

oppia / oppia