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

fix: Devise email translation customization #109

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Quentinchampenois
Copy link

Description

Current version does not allow to translate Devise emails translations (subjects, etc...)

Decidim::TermCustomizer::Context::JobContext doesn't handle the Devise job arguments format. At the moment, it expects Decidim jobs to have as argument a Decidim::Organization or Decidim::User in method resolve!.

I found that Devise mailer jobs contains a list of arguments and the expected argument in a Hash { args: [Decidim::User] } for example.

Related to

How to test

Create multiple devise translation customizations :

For account confirmation mails :

Create TermCustomizer entries

  • devise.mailer.confirmation_instructions.instruction
  • devise.mailer.confirmation_instructions.subject

Refresh cache and create a new account

For reset password confirmation :
Create TermCustomizer entries

  • devise.mailer.reset_password_instructions.subject

Refresh cache and ask a reset password for user@example.org

For new admin invitation
Create TermCustomizer entries

  • devise.mailer.invite_admin.subject

Refresh cache and invite a new admin

@Quentinchampenois Quentinchampenois marked this pull request as ready for review November 9, 2023 09:29
@AyakorK
Copy link

AyakorK commented Nov 29, 2023

Hello @Quentinchampenois, we've noticed on few applications containing this branch on their Gemfile that we had an issue with the sending of notifications due to this line

arg = arg[:args].first if arg.is_a?(Hash)

We may need to identify the reason of the issue as soon as possible and find an alternative to this fix.

Copy link
Contributor

@sinaeftekhar sinaeftekhar 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 review.
Your PR solves the issue. I left a comment.

@@ -10,6 +10,8 @@ def resolve!
# passed for the job.
user = nil
data[:job].arguments.each do |arg|
arg = arg[:args].first if arg.is_a?(Hash) && arg.has_key?(:args)
Copy link
Contributor

@sinaeftekhar sinaeftekhar Dec 29, 2023

Choose a reason for hiding this comment

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

I would suggest adding a comment(i.e with a reference to #93).
Also, I would move this line of code to organization_from_argument method, so as to affect only organization setting

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review, I convert to draft and re-open it once I implemented your suggestion !

Copy link
Author

Choose a reason for hiding this comment

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

I can move this line in organization_from_argument method but I wonder if we should also keep it for space_from_argument and component_from_argument because all these methods takes arg as argument

@Quentinchampenois
Copy link
Author

Hello, I will work on resolving conflicts asap

About the change request, should I move the line :
arg = arg[:args].first if arg.is_a?(Hash) && arg.has_key?(:args)

In the following methods : organization_from_argument, space_from_argument, component_from_argument or I keep it as it ?

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.

3 participants