FIX: When admin changes an email for the user the user must confirm the change (PR #10830)

GitHub

This is misleading because the property is staff? which can be true for admins or moderators.

I recommend renaming it to requested_by_staff?

Double braces can be confusing in this case, because one represents a block and one represents a hash. I suggest making this a multi line do ... end block to make it more clear.

This should also use && ! self_requested? – an administrator changing their own email shouldn’t get the special “was requested by someone else” message.

@riking @eviltrout I have made the review changes, and renamed the self_requested? method to requested_by_self? to make it fit in better.

Are we able to do this in a single query by joining on the email_token table?

Are we able to compare the ids here instead? Comparing the AR objects may trigger two SQL queries which we might not need in certain cases.

        EmailChangeRequest.create!(

Are we able to test this without mocks? We should try and avoid mocks and stubs whenever possible.

I think there is a mistake in the description here. It does not reflect what the test is asserting for.

Same comment as https://github.com/discourse/discourse/pull/10830/files#r500677796

We need to be careful when using not_to here. The response.status can return 500 and this test will still pass. Instead I think it is better to test for the exact response.status we expect.

Do we need to remove the following lines too? https://github.com/discourse/discourse/blob/3303b7f9d0a0e2a4cf310eadc9e4dc8fa9ed8572/app/controllers/users_email_controller.rb#L17-L18

Out of curiosity, why do we need to validate that the email_token exists in the database for this code path but not the others?

@tgxworld review changes done :slight_smile:

Hmm if the response.status is 200, we don’t need this assertion anymore since a redirection will have a response.status of 301

Is this spec correct since the context for this spec is a request not made by an admin.

I think this spec is incorrect too.

I’ll update the description here to just reflect that the email with the right body is sent.

The change was requested by the user so not_to include("This email change was requested by a site admin.") is correct