-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annual limit sending validation + budget component on ready to send screen #2010
Conversation
🧪 Review environmenthttps://ytazr6zstilj2htclckabtywo40pmqwo.lambda-url.ca-central-1.on.aws/ |
…` class when sending
…oving forward if service is at either limit
…ification_counts_client
0aeecf7
to
e3b67c1
Compare
… have the info needed for budget component
…e FF is on, show them as long as they have remaining cap today and this year
from app.models.service import Service | ||
|
||
|
||
class NotificationCounts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class 👏🏽
@@ -125,6 +126,31 @@ def get_char_limit_error_msg(): | |||
return _("Too many characters") | |||
|
|||
|
|||
def get_limit_stats(notification_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function feels like a replica for notification_counts_client.get_limit_stats which you call underneath on l#131. Can we just rename it to format_limit_stats? Just so in the future the caller knows this is being set elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is just transforming the data for easy use in this case. I can rename.
|
||
|
||
# TODO: consolidate this function and other functions that transform the results of template_statistics_client calls | ||
def _aggregate_notifications_stats(template_statistics): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is template_statistics? a dict being set from where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template_statistics is the return structure from the API. Ideally these methods would be closer to that API (i.e. in the template_statistics_client
- but it seemed like a big lift for here.
return [s for s in template_statistics if s["status"] != "cancelled"] | ||
|
||
|
||
def _aggregate_stats_from_service_api(stats): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same q as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same sort of thing. These methods were taken from various controllers and act on the return value of the API calls. It is quite messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any chance you can add that to the documentation for this class?
Summary | Résumé
This PR adds validation at notification send time to ensure:
It also updates the ready to send page to:
Related cards
Screenshots
Error messages for annual limit
Changes to template page
Budget component - capacity left
Budget component - running out of capacity
Budget component - no more left this year
Test instructions | Instructions pour tester la modification
Error messages
** Note that is not possible to have the message shown when remaining
messages = 0
, because the send buttons are now hidden in that case.Appropriate error message shown when: