This is misleading because the property is
staff? which can be true for admins or moderators.
I recommend renaming it to
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.
Are we able to do this in a single query by joining on the
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.
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.
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
Hmm if the
200, we don’t need this assertion anymore since a redirection will have a
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