Add condition to check user is current coordinator

[lalit97]

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.

Bug: T226369

[Samwalton9]

Thanks, this looks good! Do you feel confident writing a test for this behaviour? It would go below this one: https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/emails/tests.py#L165

You could base it on the ones above, piecing together the coordinator and commenting logic for a new test which checks that if a coordinator changes, the original coordinator doesn't receive an email.

If you're struggling with this at all just let me know and I can provide some more guidance on how to create the test :)

[lalit97]

Okay, I will give an update soon :)

[lalit97]

Small doubt, I was thinking that if the coordinator is changed then the new coordinator should receive the comment notification mails now # T226369. Am I right?

[lalit97]

I have tried to write a test for this but it is failing currently. Need your insight on this.

[Samwalton9]

Hi lalit, that's a great question and it actually got us thinking about the desired behaviour here. We're going to chat about it and i'll get back to you after the weekend :)

[lalit97]

Okay cool :)

[Samwalton9]

Thanks for your patience - you highlighted an issue with this that we hadn't considered!

So this is slightly confusing because we don't just have users in the coordinators group approving applications, staff also do so, but they're not linked directly to those publishers as the coordinator. As such, I think we want to replace this:

    if recent_app_coordinator and recent_app_coordinator != current_comment.user:
        if recent_app_coordinator != app.partner.coordinator:

with

    if recent_app_coordinator and recent_app_coordinator != current_comment.user and (recent_app_coordinator == app.partner.coordinator or recent_app_coordinator.is_staff):

We want to check that the user is either the partner's coordinator, or that they're a staff member.

[Samwalton9]

The test is failing based on code layout, not because of the tests. You can reformat the code (after making the change above) with docker-compose exec twlight /venv/bin/black -t py37 /app/TWLight to address this :)

[lalit97]

Hello @Samwalton9 I have gone through # T226369 again and having some more doubts

After working on test database it looks like any Coordinator can Comment or Evaluate an Application which was made for a specific Partner. Is it true?

If that is the case then I don't think we should add the condition
recent_app_coordinator == app.partner.coordinator

like what if the coordinator who made the recent changes was not the one who is assigned to that Partner. (as there is a Coordinator assigned to each Partner)

An alternate to solve the issue can be that we check for recent_app_coordinator that he/she is present into the Coordinators group before sending comment notifications by doing something like

recent_app_coordinator.groups.filter(name = "Coordinators").exists()

[Samwalton9]

After working on test database it looks like any Coordinator can Comment or Evaluate an Application which was made for a specific Partner. Is it true?

That's not the case - EvaluateApplicationView (https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/applications/views.py#L803) uses the CoordinatorOrSelf view mixin (https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/view_mixins.py#L32). It's confusingly constructed but this mixin actually checks that the coordinator is the coordinator for that publisher (https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/view_mixins.py#L76).

So the only people who can evaluate an application are staff members and the coordinator assigned to that partner :)

[lalit97]

Okay I have got your point, the links you have provided helped :)

The confusion occurred because I was testing with staff account so I was able to Evaluate all the applications.

One last thing about this is I think we need a else part to the solution which you have provided earlier

if recent_app_coordinator and recent_app_coordinator != current_comment.user and (recent_app_coordinator == app.partner.coordinator or recent_app_coordinator.is_staff):

Problem with this is let us assume Coordinator1 was the coordinator for Partner and he made a comment on an application. Now after sometime the coordinator for Partner is changed and new coordinator is Coordinator2.

Now if the Editor posts another comment then Coordinator2 should receive the comment notification email. But if we do not add a else part then it won't send the notification email to anyone because both parts of the 3rd condition will return False So overall it will return False also.

So after adding the else part it will look something like this

Do you think it will work?

[Samwalton9]

That's a great point! That sounds like a sensible way of handling this, nicely done!

[lalit97]

Thanks, I will make the discussed changes soon :)

[lalit97]

will fix this soon.

[lalit97]

Hello @Samwalton9 I have made the changes as we have discussed earlier.

If you're struggling with this at all just let me know and I can provide some more guidance on how to create the test :)

I have tried to write the test for the behaviour. But the test is failing currently.

whenever I try to access recent_app_coordinator it is returning None. Here are some of things which I have noticed which maybe the reason -

(1) Version model is showing as not registered. When checking revision.is_registered(Version) it is returning False.

(2) revision object of application does not not have uesr when code runs from test.
on printing vars of revision uesr_id is showing None.

[Samwalton9]

That's strange. Have you looked at test_comment_email_sending_4? It should be doing a similar test to the one you're writing now.

[lalit97]

Yes the first part of test written by me looks like test_comment_email_sending_4 and test_comment_email_sending_2 also.

Changes made by me in tasks.py is passing this test case and all the other one's also.

But it is failing in test written by me (test_comment_email_sending_6). No idea what is going wrong :(

[Samwalton9]

Can you commit your changes? It's hard for me to evaluate without seeing exactly what you've changed :)

[Samwalton9]

Oh, unless the latest commit is your latest local changes?

[lalit97]

Oh, unless the latest commit is your latest local changes?

Yes, I have no local changes currently.

I have mentioned some of the things which I have noticed in
#406 (comment)

Looks like Revisions are not being created on doing application.save()

[Samwalton9]

I've just spotted that my comment was pending so you hadn't seen it.

[Samwalton9]

Have you checked that the order of emails in the outbox is correct? Does the second email definitely get placed in the 2nd list position, and not get inserted at the start?

[lalit97]

Yes I have checked the order. I think 2nd email will get the 2nd position. Something similar is implemented in test_comment_email_sending_3.

The problem is not with the order but here even after changing the coordinator both the mails are going to coordinator1. I have left a comment earlier which may help you finding the reason behind this behaviour.

[jsnshrmn]

Note, this feedback is just from a read through of the pr, I haven't fiddled with this locally yet.

@lalit97 the application revisions are constructed in a hinky way that differs from just about everything else, including the comments. The user for a revision represents a request user, so if you want that value set in your test revision, you need to post to the application with a client request, like in test_comment_email_sending_4

[jsnshrmn]

if that doesn't clear things up for you, just let me know and I'll do a real review with more detailed feedback/advice.

[lalit97]

the application revisions are constructed in a hinky way that differs from just about everything else, including the comments. The user for a revision represents a request user, so if you want that value set in your test revision, you need to post to the application with a client request, like in test_comment_email_sending_4

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

(1) As you 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#L288 I think. So due to this the issue mentioned at T226369 is still not resolved.

[jsnshrmn]

I rebased this so I can check it against our current code base and review.

[suecarmol]

LGTM! Thank you for your contribution!

[lalit97]

LGTM! Thank you for your contribution!

thank you :) I think you missed my last comment #discussion_r464268579

[suecarmol]

LGTM! Thank you for your contribution!

thank you :) I think you missed my last comment #discussion_r464268579

@lalit97 Sorry I missed that comment. Can you pick up this task with a new PR? I can help you with any questions you have on this.

[lalit97]

@lalit97 Sorry I missed that comment. Can you pick up this task with a new PR? I can help you with any questions you have on this.

No problem, raised a new PR at #506 :)