feat: preview ballot email before save#9646
feat: preview ballot email before save#9646rjsparks merged 9 commits intoietf-tools:feat/ballotemailfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/ballotemail #9646 +/- ##
===================================================
Coverage ? 88.88%
===================================================
Files ? 319
Lines ? 41950
Branches ? 0
===================================================
Hits ? 37289
Misses ? 4661
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Some minor refactoring / additional validation is suggested inline. No objection to accepting this as-is and cleaning up in a future PR, though.
| if key not in post_data: | ||
| errors.append(f"{key} not found in post_data") | ||
| if len(errors) == 0: | ||
| person = Person.objects.filter(pk=post_data.get("balloter")).first() |
There was a problem hiding this comment.
Ideally should validate, at least for null characters, before using the parameters to build a query. Given the restricted access to this endpoint, it's not essential, but probably good practice. We have ietf.utils.validators.validate_no_nulls() for this purpose.
(Could also validate things through a form using data from the JSON body, but that is probably a distraction at this point)
| discussToggle($("input[name=position]:checked").val()); | ||
| </script> | ||
|
|
||
| <script> |
There was a problem hiding this comment.
Should probably refactor away from inline scripts
| addrs, frm, subject, body = build_position_email_from_dict(wanted) | ||
|
|
||
| recipient_slugs = post_data.get("cc_choices") | ||
| # Consider refactoring gather_address_lists so this isn't duplicated from there |
There was a problem hiding this comment.
Seems worthwhile - it's just separating the "to" and "cc" gathering, I think? If that's all, it looks minor enough it could be included in this PR.
| "errors": errors, | ||
| } | ||
| else: | ||
| wanted = dict() # consider named tuple instead |
There was a problem hiding this comment.
Don't object to this, but I think a dict is fine
|
I'd like to move this forward as is then - we can split some of the improvements into separate PRs, but the bigger energy should go into replacing the UX for balloting with Vue/Nuxt. |
|
I'm going to retarget this to a feature branch and deploy that so the IESG can preview the change to their workflow. |
2 participants
No description provided.