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

New association year, new secretary #422

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

tomudding
Copy link
Member

Description

It is July 11 (GH-309), so it is time for another update to the renewal emails. Update the sender in renewal emails to Jens, the secretary for 2024-2025.

I am still against having the names hard-coded in the e-mails, but as previously discussed in GH-286 we do not have a better way of doing this (yet).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation improvement (no changes to code)
  • Other (please specify)

@rinkp
Copy link
Member

rinkp commented Jul 11, 2024

GH-390 to the rescue?

In the long run that will require a frontend but in the meantime it can still be better to edit it in the database instead of in the code.

@tomudding
Copy link
Member Author

GH-390 to the rescue?

In the long run that will require a frontend but in the meantime it can still be better to edit it in the database instead of in the code.

I have been thinking about this for a few weeks, and at first, I only saw new problems. However, this will work because of the existing exception handling:

$renewalLink = $this->actionLinkMapper->createRenewalByMember($graduate);
try {
$this->sendRenewalEmail($renewalLink);
} catch (Throwable $e) {
$this->actionLinkMapper->remove($renewalLink);
throw $e;
}

I will modify this PR to move to the configuration service but will not implement a UI for it.

@rinkp rinkp marked this pull request as draft July 24, 2024 20:06
@tomudding tomudding force-pushed the feature/updated-secretary-in-mails branch from 9059fca to 0cf39b0 Compare August 7, 2024 18:23
I am against having the names hard-coded in the e-mails. We previously discussed
in GEWISGH-286 that we do not have a better way of doing this, since then we have the
new dynamic configuration available that would solve this (GEWISGH-390) but would
still require updates each year to ensure the data is up-to-date.

Instead, we do a bit less personalisation and a bit more of not hard-coding
names and dates.
@tomudding tomudding force-pushed the feature/updated-secretary-in-mails branch from 0cf39b0 to 39aadc3 Compare August 7, 2024 19:22
@tomudding
Copy link
Member Author

The 'ready for review' button just disappeared, that is rather unfortunate

@tomudding tomudding marked this pull request as ready for review August 7, 2024 19:23
@tomudding
Copy link
Member Author

Adjusted based on our earlier discussions of not wanting to hardcode anything. Moving it to the dynamic configuration was an option but still requires someone to make the appropriate changes at the right time; that is an assumption we should not want to make.

Further adjustments were made based on feedback from the board.

@tomudding tomudding requested a review from rinkp August 7, 2024 19:26
@tomudding tomudding merged commit b046723 into GEWIS:main Aug 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants