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.

Sign up for GitHub

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

Jump to bottom

PPF-57 Sent reminder email #1377

Merged
merged 65 commits into from
Sep 19, 2024
Merged

PPF-57 Sent reminder email #1377

merged 65 commits into from
Sep 19, 2024

Conversation

grubolsch
Copy link
Contributor

Added

  • Sent a mail to integrators when after a year the integration is still not activated.

Ticket: https://jira.uitdatabank.be/browse/PPF-57

Base automatically changed from PPF-53/improve-mail-manager to main September 12, 2024 12:59
@grubolsch
Copy link
Contributor Author

Also renamed the new database field to reminder_email_sent

@grubolsch
Copy link
Contributor Author

Feedback has been processed

@LucWollants
Copy link
Contributor

I wonder if the requirements change after this is implemented: https://jira.publiq.be/browse/PPF-564
Maybe we need to avoid sending emails to integrations that were put on hold by an admin.

LucWollants
LucWollants reviewed Sep 18, 2024
View reviewed changes
app/Domain/Integrations/Integration.php
@@ -53,7 +55,7 @@ public function __construct(
public readonly string $description,
public readonly UuidInterface $subscriptionId,
public readonly IntegrationStatus $status,
public readonly IntegrationPartnerStatus $partnerStatus
public readonly IntegrationPartnerStatus $partnerStatus,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public readonly IntegrationPartnerStatus $partnerStatus,
public readonly IntegrationPartnerStatus $partnerStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually choose on purpose to leave these trailing comma's in.
The idea is that if we ever add a new option you would have not to change anything on the previous line, meaning that in a PR that line would not show up making the change more clear.
Happy to change it if it is not in line with the current coding standards, but it does have a purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have enforced this rule with trailing commas on arrays but not on methods. If we want to enforce it, we could add a rule for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to create a rule for it later. Is it oké if I let it be for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

Koen Eelen and others added 11 commits September 18, 2024 15:00
remove useless constants
1bc572f
fix unit test
392e3a5
@grubolsch
Merge pull request #1392 from cultuurnet/PPF-53/sandbox-mode-expansion
2a74892 
PPF-53 Added whitelist for sandbox mode
Use name of Template consistently
9874705
@grubolsch
Merge pull request #1390 from cultuurnet/PPF-53/move-template-config-…
d57345d 
…to-seperate-class

PPF-53 Move template config to seperate class
Merge branch 'PPF-57/sent-reminder-email' into PPF-53/extra-mails
bc4ab5c
@grubolsch
Merge pull request #1391 from cultuurnet/PPF-53/extra-mails
9967cb0 
PPF-53 Added activation requested and integration deleted mail
Remark IntegrationExpirationTime into config values
8401baf
add service provider for cli command
ecb9a69
fix bug in mailjet mails field - array in array
775c8ec
@grubolsch
Merge branch 'main' into PPF-57/sent-reminder-email
c772d0e
LucWollants
LucWollants approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@LucWollants LucWollants left a comment

Choose a reason for hiding this comment

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

Thanks for all the refactors. We have a very readable and consistent implementation now for mails, which follows the same flow as for other external tools like Insightly and Keycloak.

@grubolsch grubolsch merged commit 251ea83 into main Sep 19, 2024
8 checks passed
@grubolsch grubolsch deleted the PPF-57/sent-reminder-email branch September 19, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucWollants LucWollants LucWollants approved these changes

@JonasVHG JonasVHG Awaiting requested review from JonasVHG JonasVHG is a code owner

@brampauwelyn brampauwelyn Awaiting requested review from brampauwelyn brampauwelyn is a code owner

@simon-debruijn simon-debruijn Awaiting requested review from simon-debruijn simon-debruijn is a code owner

@vhande vhande Awaiting requested review from vhande vhande is a code owner

@Anahkiasen Anahkiasen Awaiting requested review from Anahkiasen Anahkiasen is a code owner

@dev-publiq dev-publiq Awaiting requested review from dev-publiq dev-publiq is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grubolsch @LucWollants