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

Add notification for closeAccount #286

Closed
wants to merge 6 commits into from

Conversation

AlexanderWillner
Copy link

We've already all required hooks and code for sending a notification when a member gets activated. This patch adds the same functionality for the closeAccount event.

This change was implemented and tested using 1.6.14. For the pull request a diff was generated for main. A backport for 1.6.15 would be nice.

@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 14, 2023

A backport for 1.6.15 would be nice.

I am pretty sure terminal42/notification_center uses semantic versioning. New feature would go into 1.8. Why do you need it for 1.6?

library/NotificationCenter/ContaoHelper.php Outdated Show resolved Hide resolved
languages/de/tl_nc_notification.xlf Outdated Show resolved Hide resolved
languages/de/tl_module.xlf Outdated Show resolved Hide resolved
dca/tl_module.php Outdated Show resolved Hide resolved
@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 14, 2023

A backport for 1.6.15 would be nice.

I am pretty sure terminal42/notification_center uses semantic versioning. New feature would go into 1.8. Why do you need it for 1.6?

Btw. the changes in this PR could easily be integrated as an App customisation or even extension of your own, if you really need it for an older feature version.

@AlexanderWillner
Copy link
Author

I am pretty sure terminal42/notification_center uses semantic versioning. New feature would go into 1.8. Why do you need it for 1.6?

As version 1.6 is needed for setups with Haste Version 4.*

Btw. the changes in this PR could easily be integrated as an App customisation or even extension of your own, if you really need it for an older feature version.

We've many customisations, just trying to give changes back to the community.

AlexanderWillner and others added 2 commits May 14, 2023 18:43
Co-authored-by: Fritz Michael Gschwantner <fmg@inspiredminds.at>
Copy link
Sponsor Collaborator

@fritzmg fritzmg left a comment

Choose a reason for hiding this comment

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

lgtm in general :)

Two things to consider though:

The name of the new member_close_mode token does not make much sense to me. The member_* tokens are supposed to represent the properties of the member itself. However the member_close_mode represents the reg_close setting in the close account module. imho the token's name should be just close_mode for example.

The new token is also missing in the $GLOBALS['NOTIFICATION_CENTER']['NOTIFICATION_TYPE'] array. It should be added to email_subject, email_text and email_html for the member_close notification type. Plus appropriate translation labels for the new token.

@AlexanderWillner
Copy link
Author

Thanks for the feedback. I updated the patch.

@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 15, 2023

👍

The tokens.xlf still needs to be updated though with a description of thew new token.

@AlexanderWillner
Copy link
Author

Added a suggestion.

@fritzmg
Copy link
Sponsor Collaborator

fritzmg commented May 15, 2023

Did you test this change? Doesn't it need to be NOTIFICATION_CENTER_TOKEN.member_close.close_mode?

@AlexanderWillner
Copy link
Author

Did you test this change? Doesn't it need to be NOTIFICATION_CENTER_TOKEN.member_close.close_mode?

Sorry, no I did not test this change :/ I renamed it accordingly.

@Toflar
Copy link
Member

Toflar commented May 15, 2023

Thanks for the contribution! However, I won't be releasing any new 1.x version anymore. Even if I did, it would not be helpful to you as it would have to be 1.8 and thus haste 5-only anyway - I don't want to backport anything and release as bugfix release and potentially causing other issues unless it's absolutely necessary (which a new feature does not qualify for).

I would love to see the PR being picked up again after v2 is released, though. Maybe it can be a nice addition to the core then and be part of a future 2.1 or 2.x release? :)

In the meantime, feel free to publish this as your own extension which allows you to meet all your needs.

@Toflar
Copy link
Member

Toflar commented Feb 1, 2024

I'm cleaning up issues and PRs so I will have to close this PR but please, feel free to work on a PR for version 2 (to be released very, very soon) once it's here if you still care 😊

@Toflar Toflar closed this Feb 1, 2024
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.

None yet

3 participants