Skip to content

feat: split liaison_statement_posted mailtrigger into outgoing and incoming#9553

Merged
rjsparks merged 5 commits intomainfrom
unknown repository
Oct 7, 2025
Merged

feat: split liaison_statement_posted mailtrigger into outgoing and incoming#9553
rjsparks merged 5 commits intomainfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 18, 2025

Fixes #9434

@ghost ghost force-pushed the liaisons-mt-split branch from aab922e to a94cf67 Compare September 18, 2025 15:13
@rjsparks
Copy link
Copy Markdown
Member

Please put playwright/tests/meeting/floor-plan.spec.js back. If you're seeing playwright failures, they aren't related to what you're working on - call them out to us and we'll identiffy what's happening.

Then make sure migrations ran correctly in your dev instance and regenerate the names fixture with

ietf/manage.py generate_name_fixture --stdout | jq --sort-keys 'sort_by(.model, .pk)' > ietf/name/fixtures/names.json

(You can find that using ietf/manage.py generate_name_fixture --help).
and commit the updated result.

Copy link
Copy Markdown
Member

@rjsparks rjsparks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for helping. Comments inline.

Please let me know if the next set of changes aren't obvious.

)
liaison_posted_incoming.to.add(recipients_to)
liaison_posted_incoming.to.add(*recipients_cc)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should remove liaison_statement_posted

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have attempted to remove with

Mailtrigger.objects.filter(slug__in=("liaison_statement_posted")).delete() 

MailTrigger.objects.filter(
slug__in=("liaison_statement_posted_outgoing", "liaison_statement_posted_incoming")
).delete()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this should add liaison_statement_posted back

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you recommend adding this back inside reverse? NB fixture might still be marking liaison_statement_posted as active in this branch

Comment thread ietf/mailtrigger/utils.py Outdated
return mailtrigger


def get_cc(group):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method name used to rely (too much) on the file it was in for context. With this move, it should be something like get_contacts_for_liaison_messages_for_group.

This and the function after it should be merged into one that pays attention to outgoing/incoming, or perhaps one should be named ...primary... and the other ...secondary...

Whether these contacts land in the to or the cc should be up to how the mailtriggers use them, so getting the notion of cc or to out of the name is important. (See the title of #9434).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed the functions, would they still need outgoing/incoming separate from the mails logic if merged into one inside utils? CC comments need to be revised too.

Comment thread ietf/mailtrigger/utils.py Outdated

return emails

def get_contacts_for_group(group):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this should be something like get_contacts_for_liaison_messages_for_group though that's getting a bit long.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(should have edited that before posting it to match the comments above. What's said at L77 is what's intended.)

desc="Recipients for a message when a new outgoing liaison statement is posted",
)
liaison_posted_outgoing.to.add(recipients_to)
liaison_posted_outgoing.cc.add(*recipients_cc)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #8271 - lets add the new Recipient liaison_from_contact to the cc for outgoing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see the title of #9434 - lets see if we can make that move from cc to to with this migration.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added this to cc for outgoing and to for incoming, ty for noting parent issue

@ghost ghost force-pushed the liaisons-mt-split branch from c8a241c to f19a47e Compare September 19, 2025 16:24
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.87%. Comparing base (ea50ebc) to head (5900c11).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
ietf/liaisons/views.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9553   +/-   ##
=======================================
  Coverage   88.86%   88.87%           
=======================================
  Files         319      319           
  Lines       41911    41925   +14     
=======================================
+ Hits        37245    37260   +15     
+ Misses       4666     4665    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@rjsparks rjsparks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have time, please make this change, otherwise, we'll finish this out in the next few days.

slug__in=("liaison_statement_posted_outgoing", "liaison_statement_posted_incoming")
).delete()

Mailtrigger.objects.get(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a create and the Recipient m2ms will have to be repopulated.

@ghost ghost marked this pull request as ready for review October 1, 2025 13:57
@rjsparks
Copy link
Copy Markdown
Member

rjsparks commented Oct 2, 2025

I've reworked the migration a bit.

@rjsparks rjsparks merged commit a8e8b9e into ietf-tools:main Oct 7, 2025
10 checks passed
@ghost ghost deleted the liaisons-mt-split branch October 7, 2025 18:29
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When sending an incoming liaison statement, move the list address for any groups from the Cc to the To field.

2 participants