Add condition to check user is current coordinator

[lalit97]

Description

We should do a check before sending a comment notification email that the coordinator we're about to email is also the current coordinator of the partner.

Phabricator Ticket

T226369

Types of changes

• Bug fix (non-breaking change which fixes an issue)

[lalit97]

The test is passing somehow but I am stuck on couple of points. Need some help to fix these

(1) As @jsnshrmn have suggested I have made a post request after changing the coordinator (somewhat like test_comment_email_sending_4). But if I remove the post request the test fails again. So I was thinking that ideally it should work only after doing partner.save() and must not require an additional post request to create revisions.

(2) I tried to print mail.outbox[2].to and it is still showing that another mail is sent to the previous coordinator also. Which is occurring due to tasks.py#L292 I think. So due to this the issue is still not completely resolved.

[jsnshrmn]

Just now circling back around to this one. Apologies for the delay:

1. yeah, actually, this was a brain fart on my part. We moved most of our app revision code out of a view (which required using a client instead of just accessing the model) and into a post_commit signal which mostly works as you'd expect. You do still have to make sure that you set the user up correctly.

2. I'll have a look.

It looks like there's nothing in this pr but newlines? Is that just placeholder for discussion?

[jsnshrmn]

1. So, looking at the post issue: If the editor making the current revision isn't set correctly (by setting the app editor before save or doing a post to the relevant view), failure is exactly what I'd expect.
2. I've spent a fair amount of time noodling around with this, and I agree that the outbox contents are not what I'd expect. There is quite a bit of statefulness here because of our use of test client sessions, making this tricky to debug. I think a rewrite of the class setup might be in order. While that is in scope of the task in phabricator, it's pretty clear that this isn't the "good-first-task" that we thought it was. We have significant work on the test suite in the backlog anyway, so you might just want to walk away from this one, unless test design in itself is of interest to you.

[lalit97]

Just now circling back around to this one. Apologies for the delay:

No worries at all :)

It looks like there's nothing in this pr but newlines? Is that just placeholder for discussion?

Actually the changes near the newlines were merged by mistake refer #406. So I have raised a new PR with newlines near the previously made changes.

So, looking at the post issue: If the editor making the current revision isn't set correctly (by setting the app editor before save or doing a post to the relevant view), failure is exactly what I'd expect.

Oh great I was unable to think about this point :)

I've spent a fair amount of time noodling around with this, and I agree that the outbox contents are not what I'd expect. There is quite a bit of statefulness here because of our use of test client sessions, making this tricky to debug.

Yes I also tried to understand the process of revisions and all several times but it was just too complex.

We have significant work on the test suite in the backlog anyway, so you might just want to walk away from this one, unless test design in itself is of interest to you.

Okay so is there anything needed to be done in this from my side? should I close the pull request?

[jsnshrmn]

I'll just close this out. Thanks for raising the various issues that you uncovered, as it's spurred us to put some test suite cleanup on our priority list.

[lalit97]

I'll just close this out. Thanks for raising the various issues that you uncovered, as it's spurred us to put some test suite cleanup on our priority list.

Okay, my pleasure :)