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

Provide support for In-memory template manager #256

Merged
merged 8 commits into from
Aug 29, 2024

Conversation

JeethJJ
Copy link
Contributor

@JeethJJ JeethJJ commented Aug 27, 2024

Recreating PR : #254

Describe the issue:
When introducing new notification templates (Email/SMS) and migrating customers to on-premises solutions, a data migration is currently required for existing tenants. However, with the removal of data migration support in upcoming Identity Server (IS) releases, we need to address this issue to ensure smooth template integration without relying on data migration.

Proposed changes in this pull request
Currently email/sms templates are stored in the registry, for any sub tenants we are not adding new templates when server startup. When creating any tenants we are adding default email templates to the registry per tenant, this leaves redundant data. With this fix, we are providing In-memory template manager which return default templates and act on read only mode.

Currently, it works with hybrid template manager and db-based template manager also.

Issue : wso2/product-is#20996

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10573242101

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10573242101
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10577681037

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10577681037
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10577681037

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10608829457

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10608829457
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10608829457

@darshanasbg
Copy link
Contributor

darshanasbg commented Aug 29, 2024

Also with this change we dont need to persist default templates with server startup[1] and tenant creation[2].. So lets remove those and associate code that become unused with that..

In fact we can,

  • Remove whole TenantManagementListener.
  • Remove loadDefaultEmailTemplates() loadDefaultSMSTemplates() method impls
  • Have EmailTemplateManager.addDefaultEmailTemplates() method deprecated
  • Have EmailTemplateManagerImpl.addDefaultEmailTemplates() method deprecated & clean the impl to throw not implemented exception.
  • Have NotificationTemplateManager.addDefaultNotificationTemplates() method deprecated
  • Have EmailTemplateManagerImpl.addDefaultNotificationTemplates() method deprecated & clean the impl to throw not implemented exception.

[1]

// Load default notification templates.
loadDefaultEmailTemplates();
loadDefaultSMSTemplates();

[2]
templateManager.addDefaultEmailTemplates(tenantDomain);
} catch (I18nEmailMgtException e) {
String message = "Error occurred while loading default email templates for the tenant : " + tenantDomain;
log.error(message);
throw new StratosException(message, e);
}
try {
NotificationTemplateManager notificationTemplateManager = new EmailTemplateManagerImpl();
notificationTemplateManager.addDefaultNotificationTemplates(

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10612123038

@JeethJJ
Copy link
Contributor Author

JeethJJ commented Aug 29, 2024

Changes suggested through [1] will be addressed in another PR.

[1] #256 (comment)

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10612123038
Status: cancelled

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10612717489

darshanasbg
darshanasbg previously approved these changes Aug 29, 2024
@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10612717489
Status: cancelled

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/10612848361

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/10612848361
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10612848361

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.

4 participants