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

raafeahmed · 2024-04-23T08:12:44Z

Overview

This PR fixes Exception: This user does not have any role in exploration with ID #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.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
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.

… error response from the backend.

…on the frontend.

…ng the copy made earlier and display error message to end user.

oppiabot · 2024-04-23T08:12:56Z

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

seanlip

Thanks @raafeahmed, UI looks good! Left a few notes for your consideration.

seanlip · 2024-04-24T18:25:59Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -230,6 +230,20 @@ export class ExplorationRightsService {
  }

  removeRoleAsync(memberUsername: string): Promise<void> {
+    const initialState = {


Can you do this in a way that doesn't assume there will always be three categories of usernames? For example, store just the name of the field containing the username in question and remove the member username from it, then add the member username back later to the same category if needed (or reinstate the old version of the category).

Other fields should not be affected here.

Not done -- per the previous comment, I don't expect to see reference to three separate things here. Please make the code more general so that if we add a new username category, we don't have to update this code. (As a bonus, this will also reduce the repeated logic.)

Sorry, I assumed you had meant to just check which array the member username was in. The new code has been generalized to iterate through the specific categories and do the relevant operations. The only thing that would have to be updated for future is simply the first line, where you would just have to add more username categories to the categories array.

seanlip · 2024-04-24T18:27:40Z

core/templates/pages/release-coordinator-page/beam-jobs-tab/beam-jobs-tab.component.spec.ts

@@ -220,7 +220,7 @@ describe('Beam Jobs Tab Component', () => {
    expect(await table.getRows()).toHaveSize(2);

    component.ngOnDestroy();
-  });
+  }, 10000);


Which step here in particular is taking a long time? Are these tests failing on CI or your local machine?

Same question for other tests. The concern I have is that these tests aren't failing elsewhere (AFAIK) so I don't understand what is causing this timeout for this PR specifically.

The tests will fail randomly on my local machine, and is usually one or two of them that fail at a time on each run. I provided an example screenshot of what one of these errors look like in the proof of changes section in the PR. The reason I changed it was because I unfortunately couldn't get past these errors in order to push my code for the actual issue and create this PR. Modifying the timeout is what allowed me to do so.

I also ran into these timeout errors when running the original/current code repository, so it's unrelated to my changes and could be related to my local machine only. If so, is it possible for this change to omitted without me having to push the code without this timeout modification? Or if there's something else that I need to do to make it pass on my machine?

Can you investigate (perhaps by commenting out bits) which line of the test in particular is taking a long time on your machine?

Was able to push without it, so these modifications have been reverted to the original code.

seanlip · 2024-04-24T18:30:51Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -247,6 +261,14 @@ export class ExplorationRightsService {
          response.rights.community_owned,
          response.rights.viewable_if_private
        );
+      })
+      .catch(error => {


Just a thought: now that the frontend doesn't actually reflect the state of the backend, I think we need to show some confirmation to the user. Can you show a green toast in the bottom-left/bottom-right on successful deletion saying something like "Successfully removed owner/editor/viewer: {{username}}"? (I think we might already have some functionality for this sort of thing, so I'd suggest reusing it.)

seanlip · 2024-04-24T18:32:23Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    ).and.returnValue(Promise.reject(new Error('Error')));
+
+    ers.removeRoleAsync('ownerName').then(successHandler, failHandler);
+    expect(ers.ownerNames).not.toEqual(['ownerName']);


In general, try to assert equality rather than inequality (it's a stronger check).

seanlip · 2024-04-24T18:32:35Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    tick();
+
+    expect(successHandler).toHaveBeenCalled();
+    expect(failHandler).not.toHaveBeenCalled();


In the failure case, shouldn't the failHandler be called?

Hm, when i switched it it threw an error. Regardless though, the main point of the test case is to show that the list is empty before tick() meaning the user is removed, and then after tick, the list has the original value again, due to the error being returned. I'm just going to remove these two lines since it might be confusing and not exactly a relevant detail to the test case.

No, I don't think that suffices ... please check to understand whether the right thing is happening here with the handlers. I think it's a good idea to keep them because it serves as additional confirmation about what is being called in the frontend when the response returns.

Done. The error wasn't being returned properly which for some reason caused successhandler to be called and not the fail handler. Adding 'throw error' to the end of the catch block fixed it. I also put several expect statements to verify the states of the categories at every moment. Test is passing.

…ific list, and only add back user to its specific list on failure.

raafeahmed

@seanlip I have made some changes, please take another look.

raafeahmed · 2024-04-25T03:23:32Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -247,6 +261,14 @@ export class ExplorationRightsService {
          response.rights.community_owned,
          response.rights.viewable_if_private
        );
+      })
+      .catch(error => {


raafeahmed · 2024-04-25T03:23:43Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -230,6 +230,20 @@ export class ExplorationRightsService {
  }

  removeRoleAsync(memberUsername: string): Promise<void> {
+    const initialState = {


raafeahmed · 2024-04-25T03:27:52Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    tick();
+
+    expect(successHandler).toHaveBeenCalled();
+    expect(failHandler).not.toHaveBeenCalled();


Hm, when i switched it it threw an error. Regardless though, the main point of the test case is to show that the list is empty before tick() meaning the user is removed, and then after tick, the list has the original value again, due to the error being returned. I'm just going to remove these two lines since it might be confusing and not exactly a relevant detail to the test case.

raafeahmed · 2024-04-25T03:29:01Z

core/templates/pages/release-coordinator-page/beam-jobs-tab/beam-jobs-tab.component.spec.ts

@@ -220,7 +220,7 @@ describe('Beam Jobs Tab Component', () => {
    expect(await table.getRows()).toHaveSize(2);

    component.ngOnDestroy();
-  });
+  }, 10000);


Was able to push without it, so these modifications have been reverted to the original code.

raafeahmed · 2024-04-25T03:30:30Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    ).and.returnValue(Promise.reject(new Error('Error')));
+
+    ers.removeRoleAsync('ownerName').then(successHandler, failHandler);
+    expect(ers.ownerNames).not.toEqual(['ownerName']);


seanlip

Thanks, left a few more comments. Please also see #20201 (comment).

seanlip · 2024-04-25T05:30:43Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -230,6 +230,20 @@ export class ExplorationRightsService {
  }

  removeRoleAsync(memberUsername: string): Promise<void> {
+    const initialState = {


Not done -- per the previous comment, I don't expect to see reference to three separate things here. Please make the code more general so that if we add a new username category, we don't have to update this code. (As a bonus, this will also reduce the repeated logic.)

seanlip · 2024-04-25T05:32:39Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    ).and.returnValue(Promise.reject(new Error('Error')));
+
+    ers.removeRoleAsync('ownerName').then(successHandler, failHandler);
+    expect(ers.ownerNames).toEqual([]);


Verify that the other categories are not affected. Ditto below.

…s category and adding a user back to its category on failure

…fy state of each category

oppiabot · 2024-05-02T10:01:45Z

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!

raafeahmed

@seanlip Apologies for the inactivity, was finishing up finals. I've addressed your comments, and I've also updated the screen recordings, please take another look.

raafeahmed · 2024-04-25T09:42:37Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    ).and.returnValue(Promise.reject(new Error('Error')));
+
+    ers.removeRoleAsync('ownerName').then(successHandler, failHandler);
+    expect(ers.ownerNames).toEqual([]);


raafeahmed · 2024-04-25T09:48:32Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -230,6 +230,20 @@ export class ExplorationRightsService {
  }

  removeRoleAsync(memberUsername: string): Promise<void> {
+    const initialState = {


Sorry, I assumed you had meant to just check which array the member username was in. The new code has been generalized to iterate through the specific categories and do the relevant operations. The only thing that would have to be updated for future is simply the first line, where you would just have to add more username categories to the categories array.

raafeahmed · 2024-04-25T09:52:42Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.spec.ts

+    tick();
+
+    expect(successHandler).toHaveBeenCalled();
+    expect(failHandler).not.toHaveBeenCalled();


Done. The error wasn't being returned properly which for some reason caused successhandler to be called and not the fail handler. Adding 'throw error' to the end of the catch block fixed it. I also put several expect statements to verify the states of the categories at every moment. Test is passing.

seanlip

Thanks @raafeahmed, took another pass, PTAL.

seanlip · 2024-05-03T12:42:30Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

@@ -230,6 +230,14 @@ export class ExplorationRightsService {
  }

  removeRoleAsync(memberUsername: string): Promise<void> {
+    const categories = ['ownerNames', 'editorNames', 'viewerNames'] as const;
+    let initialState: {[key: string]: string[]} = {};


Try picking a different name besides "state", since we already use "state" in the exploration editor to mean something else. Perhaps initialUsernamesByCategory or similar.

That said, I'm a bit confused about why you need to maintain initialState at all. Wouldn't editing this[category] be sufficient?

this[category] is supposed to represent this.ownerNames, this.editorNames, etc when doing the lookup in the for loop. The initalUsernamesByCategory is for making a copy of the respective array prior to the deletion, and then using that copy to restore it in the catch block.

seanlip · 2024-05-03T12:43:45Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

+    const categories = ['ownerNames', 'editorNames', 'viewerNames'] as const;
+    let initialState: {[key: string]: string[]} = {};
+    categories.forEach(category => {
+      if (this[category]?.includes(memberUsername)) {


Why the question mark? If we want to be careful about handling the case where the category is non-existent then an error should be thrown if we don't expect that case to happen.

The ? is basically due to some of the unit tests not defining a category, but this doesn't mean an error has to be thrown. For instance, if only ers.editorNames is initialized, and then removeRoleAsync is called, ers.ownerNames and ers.viewerNames were never initalized. So when this if check runs, if its not defined, it will just skip over that category and check the next one for the username. In practice, on the UI side, I think that all the user categories are initialized. This statement was initially if(this[category] && this[category].includes[memberUsername]), which is equivalent.

Hm, I think in that case it would be better if you had the unit tests define whatever categories are necessary, and removed the "?".

An alternative is to have categories be defined more globally in this serivce, e.g. change this.ownerNames to this.roles.ownerNames and similarly for the others, and define categories as the keys of this.roles. Then tests can specify whatever they like for this.roles and it should "just work". But this might be more of a refactor than you'd want to take on, in which case please see the first paragraph.

@seanlip So it appears that some of the other frontend tests failed when I removed the "?". Specifically, in settings-tab.component.spec.ts, as some of the tests call removeRole, for instance ,

component.removeRole('username', 'editor');

removeRole will then call removeRoleasync, and since only the editor category was created here originally, this causes the same error to be thrown (ownerNames, viewerNames would be undefined). Would it be simpler to just keep the "?" instead, as I'm not sure I could define these categories before component.removeRole() is called.

No, I don't think we should be making that change just for tests. Why can't you define the necessary categories in the test?

seanlip · 2024-05-03T12:46:13Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

+        );
+      })
+      .catch(error => {
+        categories.forEach(category => {


You probably don't need to store the entire initialState, just store the category for which the user was removed, and you can then reference that again here.

Better still -- modify this function so that the category is passed into it (in addition to the username). That way, I think you'll have much simpler logic in this function.

Ok, I'm storing just the specific category, which is then referenced in the catch block.

…from, use as a reference in catch block

raafeahmed

@seanlip PTAL. Lines 235 and 236 in exploration-rights.service.ts are due to a 'no index signature found' compile error related to the userRole variable being used in the catch block. These two lines fix that error.

seanlip

Thanks -- sorry for the late reply, got swamped the past few days. Only 2 things left, PTAL!

seanlip · 2024-05-09T17:39:48Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

+    const categories = ['ownerNames', 'editorNames', 'viewerNames'] as const;
+    let initialUsernamesByCategory: string[];
+    type Category = (typeof categories)[number];
+    let userRole: Category;


Suggest roleOfRemovedUser (for clarity)

seanlip · 2024-05-09T17:40:12Z

core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts

+    const categories = ['ownerNames', 'editorNames', 'viewerNames'] as const;
+    let initialState: {[key: string]: string[]} = {};
+    categories.forEach(category => {
+      if (this[category]?.includes(memberUsername)) {


Hm, I think in that case it would be better if you had the unit tests define whatever categories are necessary, and removed the "?".

An alternative is to have categories be defined more globally in this serivce, e.g. change this.ownerNames to this.roles.ownerNames and similarly for the others, and define categories as the keys of this.roles. Then tests can specify whatever they like for this.roles and it should "just work". But this might be more of a refactor than you'd want to take on, in which case please see the first paragraph.

raafeahmed

@seanlip Edited the test that was failing previously, PTAL.

seanlip

LGTM, thanks @raafeahmed! Congrats on your first Oppia PR :)

@Lawful2002 could you PTAL as codeowner? Thanks.

oppiabot · 2024-05-14T16:26:42Z

Unassigning @Lawful2002 since they have already approved the PR.

oppiabot · 2024-05-14T16:26:46Z

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!

raafeahmed added 7 commits April 22, 2024 20:40

Make a copy of the list of users prior to deletion in the event of an…

bdac449

… error response from the backend.

Remove user from list of users as soon as delete action is confirmed …

ee3ee9a

…on the frontend.

Redisplay list of users based on values prior to deletion attempt usi…

24d721e

…ng the copy made earlier and display error message to end user.

Test if user is removed from UI before updating with backend data.

777a3fb

Test if user is removed from UI before updating with backend data.

d28e618

Test if user is redisplayed when an error occurs from backend.

a3e3031

Fix Beam Jobs Tab async timeout errors.

e17bc53

raafeahmed requested a review from a team as a code owner April 23, 2024 08:12

raafeahmed requested a review from Lawful2002 April 23, 2024 08:12

oppiabot bot assigned Lawful2002 Apr 23, 2024

seanlip requested changes Apr 24, 2024

View reviewed changes

seanlip assigned raafeahmed Apr 24, 2024

raafeahmed added 7 commits April 24, 2024 18:18

Merge upstream/develop with remove-user-role

b4c05b9

Address review comment: modify code to only remove user from its spec…

a8c4b04

…ific list, and only add back user to its specific list on failure.

Address review comment: Fix unit tests.

a80b407

Display success message to user upon successful deletion.

bac0bae

remove async timeout change

d141985

Fix linter errors

43c2726

fix compilation errors

3a33849

raafeahmed commented Apr 25, 2024

View reviewed changes

raafeahmed requested a review from seanlip April 25, 2024 03:38

raafeahmed changed the title ~~Fix #19644: Remove user role from UI, redisplay list of users in the event of a backend failure, fix beam jobs tab async timeout errors~~ Fix #19644: Remove user role from UI, redisplay list of users in the event of a backend failure Apr 25, 2024

seanlip requested changes Apr 25, 2024

View reviewed changes

raafeahmed added 5 commits April 25, 2024 04:05

merge upstream/develop with remove-user-role

bbcd036

Address review comment: Generalize the code for removing user from it…

204c485

…s category and adding a user back to its category on failure

Fix unit test to expect fail handler to be called on failure and veri…

76a68df

…fy state of each category

Fix compile errors

6e074a4

fix compile issues

749fb02

raafeahmed and others added 2 commits May 2, 2024 23:44

merge upstream/develop

e2aeb40

Merge branch 'oppia:develop' into remove-user-role

f8d575b

oppiabot bot removed the stale label May 3, 2024

raafeahmed added 2 commits May 2, 2024 23:48

Fix compilation issues

acdcdd2

merge branches

40d630f

raafeahmed commented May 3, 2024

View reviewed changes

raafeahmed requested a review from seanlip May 3, 2024 08:00

seanlip requested changes May 3, 2024

View reviewed changes

raafeahmed added 2 commits May 8, 2024 17:22

merge upstream/develop with remove-user-role

034d6e7

Address review comment: store the category for which user is removed …

ef41112

…from, use as a reference in catch block

raafeahmed commented May 8, 2024

View reviewed changes

raafeahmed requested a review from seanlip May 8, 2024 21:40

seanlip requested changes May 9, 2024

View reviewed changes

raafeahmed added 3 commits May 9, 2024 21:47

Address review comments: fix unit tests and change variable name

d32ee6e

merge upstream/develop

b54c8d9

Fix TypeError issues

8376e8e

raafeahmed commented May 13, 2024

View reviewed changes

raafeahmed requested a review from seanlip May 13, 2024 07:00

seanlip approved these changes May 13, 2024

View reviewed changes

Lawful2002 approved these changes May 14, 2024

View reviewed changes

Lawful2002 added this pull request to the merge queue May 14, 2024

oppiabot bot unassigned Lawful2002 May 14, 2024

oppiabot bot added the PR: LGTM label May 14, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 14, 2024

seanlip added this pull request to the merge queue May 14, 2024

Merged via the queue into oppia:develop with commit e364c9b May 14, 2024
82 checks passed

raafeahmed deleted the remove-user-role branch May 14, 2024 19:40

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

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

Conversation

raafeahmed commented Apr 23, 2024 • edited

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Apr 23, 2024

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafeahmed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented May 2, 2024

raafeahmed left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafeahmed May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafeahmed left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raafeahmed left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented May 14, 2024

oppiabot bot commented May 14, 2024

raafeahmed commented Apr 23, 2024 •

edited

seanlip Apr 25, 2024 •

edited

seanlip Apr 25, 2024 •

edited

raafeahmed left a comment •

edited

raafeahmed May 10, 2024 •

edited