Skip to content
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

[Feature] Notification - application status changed #10372

Merged
merged 15 commits into from
May 22, 2024

Conversation

petertgiles
Copy link
Contributor

@petertgiles petertgiles commented May 14, 2024

πŸ€– Resolves #9642

πŸ‘‹ Introduction

This branch adds a new notification - application status changed - for both in-app and email.

πŸ•΅οΈ Details

Add any additional details that could assist with reviewing or testing this PR.

πŸ§ͺ Testing

  • set your GCNotify API key (team) and template IDs
  • Rebuild and log in
  • Setup your applicant user
    • enable in-app and email notifications for the APPLICATION_UPDATE notification family
    • set your email address to your TBS email
    • set your language preference
  • submit an application to a pool
  • as an admin user, set that application to a "final decision" or "removed" status
  • ensure the applicant received an email and an in-app notification

πŸ“Έ Screenshot

image
image

🚚 Deployment

  • add new environment variables for the two new templates:
    • GCNOTIFY_TEMPLATE_APPLICATION_STATUS_CHANGED_EN
    • GCNOTIFY_TEMPLATE_APPLICATION_STATUS_CHANGED_FR

@petertgiles
Copy link
Contributor Author

@gobyrne or @marc-donofrio could you please review the email templates in GCNotify?

@petertgiles petertgiles requested a review from gobyrne May 14, 2024 13:21
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 38.40%. Comparing base (c444521) to head (f13a31e).
Report is 7 commits behind head on main.

Files Patch % Lines
apps/web/src/hooks/useNotificationInfo.ts 0.00% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10372      +/-   ##
============================================
+ Coverage     37.10%   38.40%   +1.30%     
- Complexity     1332     1345      +13     
============================================
  Files           980      984       +4     
  Lines         29952    30140     +188     
  Branches       6489     6500      +11     
============================================
+ Hits          11113    11575     +462     
+ Misses        18804    18391     -413     
- Partials         35      174     +139     
Flag Coverage Ξ”
integrationtests 70.46% <100.00%> (+0.54%) ⬆️
unittests 31.59% <33.33%> (+1.41%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@petertgiles petertgiles added the deployment Requires a change during deployment label May 15, 2024
@petertgiles petertgiles requested review from tristan-orourke and removed request for gobyrne May 17, 2024 15:05
*/
public function toGcNotifyEmail(User $notifiable): GcNotifyEmailMessage
{
$locale = $this->locale ?? $notifiable->preferredLocale();
Copy link
Member

Choose a reason for hiding this comment

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

Where does $this->locale come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The notification can have its locale explicitly set when it is created.
https://laravel.com/docs/10.x/notifications#localizing-notifications

Copy link
Member

@tristan-orourke tristan-orourke left a comment

Choose a reason for hiding this comment

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

Wonderful! Works as expected.

I noticed one little hicough, but I think its out of scope for this one. When I clicked the in-app notification, which links to http://localhost:8000/en/applicant#track-applications-section, and I was already on the page http://localhost:8000/en/applicant, then I wasn't scrolled to the correct section.

@petertgiles petertgiles added this pull request to the merge queue May 22, 2024
Merged via the queue into main with commit 0c04b6e May 22, 2024
9 of 10 checks passed
@petertgiles petertgiles deleted the 9642-notification-application-status-changed-part2 branch May 22, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment Requires a change during deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

❗[Notification] Application Status Changed
3 participants