I think this happens because there's a 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.

I think the best way to fix this is to remove the user from the list of users in the frontend as soon as the delete action is confirmed in the frontend. Then if a success response comes back from the backend, do nothing, and if an error response comes back from the backend, show an error message to the end user and redisplay the list of users based on the value before the change.

Hi @seanlip , if this issue is still available then I would like to work on it. Can you please assign it to me?

@snehalchaudhari98 Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

Please also follow the other instructions on that wiki page if you have not yet done so. Thanks!

Hi @seanlip , thanks for sharing the resources. I will add the suggested six and make sure to follow the said guidelines. Thanks for these resources.

Since the error occurs when a user role is different from the id, I think this error is caused by altering permissions on non-existent users. Basically, when the user is deleted, it should be removed from this database entirely.

One way to fix this to add error checking in the editor.py file, so that this function is skipped when the user doesn’t exist in the list of current users. Basically, I will alter the function, so that if the error condition criteria is met, then the function doesn’t output anything.

@Sadasiva20 Suppressing the error is not the right way to go. See #19644 (comment) for some more context on when this issue occurs and how to prevent it.

Also, note that this is not deleting a user; it is removing their rights to perform operations on an exploration.

Also, just as a note -- if you want to tackle this issue, please provide a clear explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.), per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue. If it looks good, we can assign you to this issue. Thanks!

How do I reproduce this issue? When I attempted to delete a user, it returns without an error

@raafeahmed That's a good question -- here is my current guess for reproducing it. Go to the exploration editor page and delete a user, then immediately try to delete that user again. Since there is a delay in the deletion of users, the second call fails and (I think) leads to the error mentioned.

Could you please verify that this is the case for you, locally? If so then #19644 (comment) might suggest a good way to proceed next.

@seanlip I was able to reproduce the error a few times over several attempts. The delay seems to happen periodically. I will take a look at the previous comments and suggest an approach to fix the issue.

Ok, here is my proposed fix to resolve the issue:

Initially, I went to /core/domain/rights_manager.py to the deassign_role function and added in a delay, time.sleep(2). This caused the delay to occur every time a deletion occurred, allowing me to reproduce the error 100% of the time. Going off of @seanlip approach, the best way to fix this is to immediately remove the user from the interface while the deletion on the backend continues to process. By doing so, the user will never have the ability to delete a role twice, which is the issue in the first place.

To implement this, I would modify core/templates/pages/exploration-editor-page/services/exploration-rights.service.ts, in the removeRoleAsync function. I will modify this file due to the fact that array objects representing manager, collaborator, and playmaker are within this file. This function currently updates these arrays once the response comes back from the backend.

First, I will make a copy of the UI state prior to the deletion. This is for in the event of an error from the backend or if the deletion was unsuccessful. Next, I will immediately remove the user role from the frontend. After that the current code for the backend deletion will run, but I will also add a catch block with this to account for an unsuccessful deletion. If the catch block executes, the arrays are reverted to their previous states using the copy that was made earlier, and finally print a message to the user saying the user couldn't be removed.

I have written the code that does this, and tested it on the development server. The user is immediately removed from the interface, and to confirm this, i made the delay even longer when testing (time.sleep(10)). To test whether the UI reverts to the original state in the event of an error, I wrote some code that forcefully throws an error, causing the catch block to run. It displays the message to the user, and the list of users remains unchanged.

Let me know if you have any questions or concerns about this, or if the issue can be assigned to me.

@raafeahmed This sounds great, thanks! Assigning to you. When can you create a PR?

@seanlip I'm failing some of the unit tests when I went to push my code, and I'm having trouble figuring out how to fix it. Here is the error I'm getting:

And here is the changes I made:

removeRoleAsync(memberUsername: string): Promise {
/* MY CHANGES */
//making copy of UI before deletion
const initialState = {
ownerNames: [...(this.ownerNames || [])],
editorNames: [...(this.editorNames || [])],
viewerNames: [...(this.viewerNames || [])],
};
//immediately removes the user from list of users
this.ownerNames = (this.ownerNames || []).filter(
name => name !== memberUsername
);
this.editorNames = (this.editorNames || []).filter(
name => name !== memberUsername
);
this.viewerNames = (this.viewerNames || []).filter(
name => name !== memberUsername
);

/* CURRENT CODE. /
return this.explorationRightsBackendApiService
.removeRoleAsyncDeleteData(
this.explorationDataService.explorationId,
memberUsername
)
.then((response: ExplorationRightsBackendData) => {
this.alertsService.clearWarnings();
this.init(
response.rights.owner_names,
response.rights.editor_names,
response.rights.voice_artist_names,
response.rights.viewer_names,
response.rights.status,
response.rights.cloned_from,
response.rights.community_owned,
response.rights.viewable_if_private
);
})
/ MY CHANGES */
//reverts to previous UI values if deletion was unsuccessful
.catch(error => {
this.ownerNames = initialState.ownerNames;
this.editorNames = initialState.editorNames;
this.viewerNames = initialState.viewerNames;
this.alertsService.addWarning(
'Failed to remove the user role. Please try again.'
);
});
}

The lines of code above the return statement and the catch block are what I added. Testing the functionality on the development server still works as expected. I'm really not sure what to make of this error, and couldn't find the tests pertaining to this. Any advice on how to fix this?

@raafeahmed Just a tip for running test before pushing code.

You don't have to run all the frontend test (we have like 9k+ test cases) which will take a lot of time. instead follow this guide to run test for updated file only.

say you have made changes to abc.ts file so you will need to update the test for this file say present at abc.spec.ts and to run frontend test for this file only. got to core/templates/combined-tests.spec.ts and update the context field to only run frontend test for this abc file.

example:

const context = require.context(
  '../../',
  true,
  /abc.spec.ts/
);

use command python -m scripts.run_frontend_tests to run the test.

once you have passed all the test cases, undo the changes in core/templates/combined-tests.spec.ts and then push your code.

you can learn more about frontend test from here

can you run the test for updated file only and share the full error you are getting in the terminal?

Thanks!

@AFZL210 Ok so I changed the context to be /exploration-rights.service.spec.ts/ in my case, and all the tests passed for this file.

However, if I undo this change, and then try to push my code, it runs the 9k+ test cases and again results in some failures.

Also, I ran python -m scripts.run_frontend_tests against the current code not including my changes, and it still throws the same/similar errors related to beam jobs tab component so these failures don't appear to be related to the changes I made. I'm still stuck on how to get past this, and I can't create a PR since I can't push the code to my repo.

Also, I ran python -m scripts.run_frontend_tests against the current code not including my changes, and it still throws the same/similar errors related to beam jobs tab component so these failures don't appear to be related to the changes I made. I'm still stuck on how to get past this, and I can't create a PR since I can't push the code to my repo.

Can you delete the current develop branch and create a new one? Follow these steps:

1. Go to a dummy branch git checkout -b dummy-branch
2. Delete develop git branch -D develop
3. Pull and checkout develop branch git checkout -b origin origin/develop

And now try running the front-end test.

And can you please create a thread in discussion also so other folks can also try to help.

Thanks!