Feat(editor): User account deletion endpoint #843
Conversation
There was a problem hiding this comment.
Looks great, thanks for doing this !
Considering the importance of this endpoint, we definitely want some tests to ensure that the authorization is covered, in case of changes in the future we don't want this to break silently.
It means mocking the http request in the tests with superagent
We also want a separate test to fetch a deleted user after the endpoint was called succesfully, and ensure all their info is wiped.
There was a problem hiding this comment.
OK, I did a thorough review, and good on us because I missed a bunch of things ! :p
Thanks a lot for writing tests, that allowed me to think about the code more thoroughly and project into specific scenarios.
I took special care with the review on this one, just to ensure nothing can go wrong :)
src/server/routes/auth.js
Outdated
| // Setting up mocking agent for test | ||
| // eslint-disable-next-line node/no-process-env | ||
| if (process.env.NODE_ENV === 'test') { | ||
| mock(request, config); |
There was a problem hiding this comment.
Does this need to be in this file? Could it not be set up at the beginning of the test file instead?
There was a problem hiding this comment.
Placing it inside the test file also seems to work, though i initially thought it needs to be in same lexical scope.
test/src/server/routes/auth.js
Outdated
| expect(res.ok).to.be.true; | ||
| const editor = await new Editor({id: userEditorId}).fetch(); | ||
| expect(editor).to.exist; | ||
| expect(verifyDeletedUser(editor.toJSON())).to.be.true; |
There was a problem hiding this comment.
We can probably replace verifyDeletedUser with a direct comparison between the editor json object and deletedUserAttrib, just for simplicity of reading the code.
We'll need to omit a couple of properties that will change between runs, as well as add a couple to deletedUserAttrib:
| expect(verifyDeletedUser(editor.toJSON())).to.be.true; | |
| const editorJson = _.omit(editor.toJSON(),['activeAt','createdAt','typeId']) | |
| expect(editorJson).to.equal(deletedUserAttrib); |
And to add to deletedUserAttrib:
"id": 2,
"name": "Deleted User #2",
"reputation": 0
superagent-mock-config.js
Outdated
| @@ -0,0 +1,41 @@ | |||
| /* eslint-disable camelcase */ | |||
There was a problem hiding this comment.
This file is currently placed in the repo root, we'll want it in test/test-helpers instead.
test/src/server/routes/auth.js
Outdated
| const otherEditorName = 'NormalUser2'; | ||
| let agent; | ||
| before(async () => { | ||
| await createEditor(adminEditorId, {name: adminEditorName}); |
There was a problem hiding this comment.
This user shouldn't be necessary.
The UserDeleter special user is in the MusicBrainz database, not in the BookBrainz one, and we don't really interact with MB other than authenticate it
src/server/routes/auth.js
Outdated
| return res.status(404).send(); | ||
| } | ||
| // deleting all user info | ||
| const deletedUser = `Deleted User#${editor.get('id')}`; |
There was a problem hiding this comment.
| const deletedUser = `Deleted User#${editor.get('id')}`; | |
| const deletedUser = `Deleted User #${editor.get('id')}`; |
src/server/routes/auth.js
Outdated
| editor.set('revisions_applied', 0); | ||
| editor.set('revisions_reverted', 0); | ||
| editor.set('total_revisions', 0); | ||
| editor.set('metabrainz_user_id', null); |
There was a problem hiding this comment.
I need to think about this, not sure if we want to keep this to be able to link it back to an MB account.
I'm thinking for example for abusive accounts that would get deleted by admins across the platforms, it could be necessary to have this information.
src/server/routes/auth.js
Outdated
| editor.set('name', deletedUser); | ||
| await editor.save(); | ||
| // eslint-disable-next-line camelcase | ||
| return res.send({metabrainz_user_id: USER_DELETER_MBID, sub: USER_DELETER}); |
There was a problem hiding this comment.
Probably no need to return any actual data. Looking at the description of https://tickets.metabrainz.org/browse/MBS-9680 and the comments there, MB only expects a 200 status code if all went well.
test/src/server/routes/auth.js
Outdated
| await createEditor(userEditorId, {name: userEditorName}); | ||
| await createEditor(otherEditorId, {name: otherEditorName}); |
There was a problem hiding this comment.
I'm not sure I understand why we need two more users here.
If I remember correctly, when you use agent = await chai.request.agent(app); below it should run the superagent queries authenticated as our mock-user with id 123456.
Meaning your test "Normal User should not be able to delete other users" is actually testing that user 123456 is not authorized, which is completely valid but does not require the extraneous users
There was a problem hiding this comment.
Yep, you are correct. we only need one user ( that we test for deletion) else only those ids would suffice.
| expect(res.ok).to.be.false; | ||
| // unauthorized | ||
| expect(res.status).to.be.equal(401); | ||
| }); |
There was a problem hiding this comment.
I'd also love to see, for good measure, another test to ensure we get a 401 for an unauthenticated user.
Basically the same test as the above, but using await chai.request(app).post(… <- this will not use a user session because we won't call /cb as we do in the before function
test/test-helpers/create-entities.js
Outdated
| @@ -121,7 +121,7 @@ export function createEditor(editorId) { | |||
| editorAttribs.id = editorId || random.number(); | |||
| editorAttribs.genderId = gender.id; | |||
| editorAttribs.typeId = editorType.id; | |||
| editorAttribs.name = internet.userName(); | |||
| editorAttribs.name = otherEditorAttribs?.name || internet.userName(); | |||
There was a problem hiding this comment.
Considering I'm already making you change something here (using metabrainzUserId instead of name) let's make it easier to be reusable:
After defining sane defaults, we can merge editorAttribs and otherEditorAttribs and pass the result to new Editor(…
Maybe the simple new Editor({...editorAttribs,…otherEditorAttribs}) ?
Problem
BB-260: Implement the user account deletion endpoint
Solution
Added post route /delete-user to remove account from BB project