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

Delete SMTP Server #256

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Delete SMTP Server #256

merged 1 commit into from
Nov 17, 2023

Conversation

leostera
Copy link
Contributor

Hello folks! 👋🏼 Here's a PR sketching support for deleting SMTP servers.

It includes:

  • Add SmtpDeleted Event
  • Add repo support for deleting rows from pool_smtp table by id
  • Add Smtp.Delete command
  • Add Smtp.Access guard
  • Add Smtp.delete controller
  • Add /admin/settings/smtp/:id/delete route
  • Add button group and delete button on Smtp index page
  • Add I18n key and translations for the delete confirmation message

Missing a few tests, and probably isn't entirely fitting the repo-style yet. I have some comments but we can discuss them over a call first.

Happy to address/amend anything, just let me know 🙌🏼

pool/app/email/repo/repo_sql.ml Outdated Show resolved Hide resolved
pool/app/email/event.ml Outdated Show resolved Hide resolved
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
pool/cqrs_command/smtp_command.ml Outdated Show resolved Hide resolved
pool/cqrs_command/smtp_command.ml Outdated Show resolved Hide resolved
pool/web/handler/admin_settings_smtp.ml Outdated Show resolved Hide resolved
pool/web/handler/admin_settings_smtp.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@mabiede mabiede 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 your PR, looks great.

Just added some spelling issues to be consistent. Feel free to mark the PR as ready 🚀 .

pool/app/pool_common/locales/i18n_de.ml Outdated Show resolved Hide resolved
pool/app/pool_common/locales/i18n_en.ml Outdated Show resolved Hide resolved
pool/web/handler/admin_settings_smtp.ml Outdated Show resolved Hide resolved
@leostera leostera marked this pull request as ready for review November 13, 2023 11:32
@leostera
Copy link
Contributor Author

leostera commented Nov 13, 2023

I'll create an integration test for the SMTP:

  • create email servers
  • delete email servers

@leostera
Copy link
Contributor Author

leostera commented Nov 17, 2023

@mabiede note that parts of #257 are here, since I needed those to write/run the tests. The last commit (dd7ea95) implements the integration tests.

@leostera leostera force-pushed the feat/delete-smtp-servers branch 4 times, most recently from 01ac59f to 0c25049 Compare November 17, 2023 12:49
fix: clear email worker cache when deleting SMTP servers
feat: add route for deleting smtp servers from root app
chore: delete unused type
test: update and delete smtp servers

In order to test the system events created in it, we need to either
loosen up the comparison relation (so that we can skip comparison of
system events by id), or allow for fixing the system event ids that will
be used so the assertions are meaningful.

Also the update test has been reintroduced after fixing this.

fix: casing/typos on email server i18n
test: add integration tests for smtp auth
Copy link
Contributor

@mabiede mabiede left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for all adjustments!

@mabiede mabiede merged commit bc0d6e0 into uzh:main Nov 17, 2023
4 of 5 checks passed
@leostera leostera deleted the feat/delete-smtp-servers branch November 17, 2023 15:38
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