Conversation
When I make changes to the notify field, now I only have to change it in one place.
The start_review_as_secretariat and start_review_as_stream_owner methods were virtually identical. Consolidate them using a few conditional checks. Now the logic isn't duplicated, so we won't need to perform maintenance in two places.
This will: - Find the stream managers by calling Recipient.gather() - extract just their email addresses - Split the 'notify' field by commas - Remove any item in the notifications list that matches any of the stream manager emails. Yes, this will incorrectly remove email addresses like: "rfc-ise@rfc-editor.org"<other_addy@example.com> Anyone who enters that gets what they deserve.
Because the document name isn't available to the form, so I can't retrieve the document in the form's `clean()` method, so I can't discover the correct document stream in the form.
I expect that this will always be satisfied, but it's a good assumption validation.
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
I have a bunch of nit picky suggestions plus one real question.
| ad_strpk = str(Person.objects.get(name='Areað Irector').pk) | ||
| state_strpk = str(State.objects.get(used=True, slug='needshep', type__slug='conflrev').pk) | ||
| self.client.login(username="secretary", password="secretary+password") | ||
| """ This set of notification recipients should cover a bunch of cases for this test. |
There was a problem hiding this comment.
This should really be a comment - triple quotes aren't meant for multiline comments, notwithstanding their use as a docstring after a function def.
|
|
||
| # Get the email addresses which should remain | ||
| other_emails = [manager_emails[stream] | ||
| for stream in list(manager_emails) |
There was a problem hiding this comment.
Pythonic preference would be [mgr_email for stream, mgr_email in manage_emails.items() if..]. If you prefr as-is, you can drop the list. (A dict acts as an iterator over its keys.)
| def start_review(request, name): | ||
| if has_role(request.user,"Secretariat"): | ||
| return start_review_as_secretariat(request,name) | ||
| """Start the conflict review process, setting the initial |
There was a problem hiding this comment.
Docstring format should ideally be:
def somefunc():
'''Do the thing this method does in a single line
After a blank line, describe in more detail / document parameters, etc.
'''|
|
||
| notify = cleaned_data['notify'] | ||
| stream_managers = [ | ||
| re.sub('([^<]*<)?(.*)(>.*)', '\2', r) |
There was a problem hiding this comment.
Could use email.utils.parseaddr() here - it'll return a tuple, (realname, email). If you apply that to addresses in the notifications list when filtering, I think that'd get rid of the caveat (although I 100% agree they get what they deserve if you choose to keep it simple)
| if doc_to_review.stream.slug not in ['ise', 'irtf']: | ||
| notify_addresses.extend( | ||
| [r.formatted_email() | ||
| for r in Role.objects.filter(group__acronym=doc_to_review.stream.slug, name='chair')] |
There was a problem hiding this comment.
Does this match the addresses now being filtered out from the notify list? Here, it's addresses for 'chair' Roles. The filtered items are 'stream_managers' from Recipients. Do these match up in the right way?
Instead of avoiding adding document stream chair addresses as a currently accurate proxy for the stream editor addresses, use the gather_stream_managers() function to find the exact addresses to avoid and remove those from any address we consider.
|
Back at ya! |
3 participants
These changes will prevent the stream editor email address from being included in the
Notice emailsfield during the creation of a conflict review. This only works for documents in the IRTF or ISE streams.Notice emailsfield has been updated to say that the stream editors are automatically notified.While I was in the code, I refactored a bit:
clean_notify()validation method, which eventually moved to the view code instead. However, the refactoring was left.start_review_as_secretariat()andstart_review_as_stream_owner()methods are now merged. Ironically, this resulted in the exact same number of lines of code, but at least there is only one method to update going forward.Fixes ietf-tools#3638.