Add DB Based Template Management Support #227

darshanasbg · 2024-03-26T10:25:53Z

Proposed changes in this pull request

$subject

When should this PR be merged

[Please describe any preconditions that need to be addressed before we
can merge this pull request.]

Follow up actions

[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

Yoshani · 2024-03-27T05:19:25Z

...o2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/DBBasedEmailTemplateManager.java

+        }
+
+        try {
+            notificationScenarioDAO.addNotificationScenario(emailTemplateTypeDisplayName, emailTemplateTypeDisplayName,


Are we passing the same param twice since the UUID decision is pending?

Improved with https://github.com/wso2-extensions/identity-event-handler-notification/pull/230/files#diff-f5880581b13fef821d04c7ace573dfa7cd6d418e6c21868874e841d674c2e065R42. First parameter used as the (case-insensitive) key and second parameter used as the display name.

CLAassistant · 2024-03-28T18:11:25Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

madurangasiriwardena · 2024-04-01T04:35:51Z

...gt/org.wso2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/util/I18nEmailUtil.java

@@ -232,4 +245,203 @@ public static String prependOperationScenarioToErrorCode(String exceptionErrorCo
        }
        return exceptionErrorCode;
    }
+
+    public static List<EmailTemplate> convertToEmailTemplates(List<NotificationTemplate> notificationTemplates) {


Add a doc comment

Fixed with https://github.com/wso2-extensions/identity-event-handler-notification/pull/229/files#diff-3df0155326aacb175c43e84b66c418b77df5791aa94329642ad61620146b4be1R734

madurangasiriwardena · 2024-04-01T04:37:42Z

...gt/org.wso2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/util/I18nEmailUtil.java

+        String[] whiteListPatterns = {TEMPLATE_REGEX_KEY};
+        String[] blackListPatterns = {REGISTRY_INVALID_CHARS};


Shall we change the variable names here?

Fixed with: bb2c28d

madurangasiriwardena · 2024-04-01T04:40:44Z

...gt/org.wso2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/util/I18nEmailUtil.java

+            throw new NotificationTemplateManagerClientException(errorCode,
+                    I18nMgtConstants.ErrorMessages.ERROR_CODE_EMPTY_LOCALE.getMessage());
+        }
+        // Regex check for registry invalid chars.


When we move out of registry, these lines might not make sense. Though the allowed characters still makes sense, at some point we have to update the comments, variables and constants.

Agreed.. Since the current implementation has a registry based impl, it will be better to keep this.. But this has to be removed when we totally remove the registry dependency. Will keep track this as a part of that task.

madurangasiriwardena · 2024-04-01T04:42:02Z

pom.xml

@@ -428,6 +433,10 @@
        <carbon.kernel.carbon.base.pkg.version>[1.0.0, 2.0.0)</carbon.kernel.carbon.base.pkg.version>
        <axis2.osgi.version.range>[1.6.1, 2.0.0)</axis2.osgi.version.range>

+        <!--Carbon Database Utils Version-->
+        <org.wso2.carbon.database.utils.version>2.1.0</org.wso2.carbon.database.utils.version>
+        <org.wso2.carbon.database.utils.version.range>[2.0.0,2.2.0)</org.wso2.carbon.database.utils.version.range>


If we want to the 2.1.0 version, shall we restrict the range to start with 2.1.0?

Fixed with: https://github.com/wso2-extensions/identity-event-handler-notification/pull/230/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R444

madurangasiriwardena · 2024-04-01T04:49:07Z

...rbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/internal/I18nMgtServiceComponent.java

            // Register EmailTemplateManagerImpl.
            ServiceRegistration notificationManagerSR = bundleCtx
-                    .registerService(NotificationTemplateManager.class.getName(), emailTemplateManager, null);
+                    .registerService(NotificationTemplateManager.class.getName(), notificationManager, null);


EmailTemplateManagerImpl implementes both EmailTemplateManager and NotificationTemplateManager interfaces. Does it makes sense anymore?

Yes.. As of now these both needs to be implemented since NotificationTemplateManager don't have enough functionalities to deprecate the EmailTemplateManager and EmailTemplateManager is not general enough to handle different notification channel types.

This is mainly due to NotificationTemplate management is not fully complete. We will have to work on wso2/product-is#9762 and get NotificationTemplateManager improved to have all needed functionalities and retire EmailTemplateManager..

At that point it should be possible to EmailTemplateManagerImpl to implement only NotificationTemplateManager.. or even more general terms move out from EmailTemplateManagerImpl by having more general notification management service.

madurangasiriwardena · 2024-04-01T04:51:28Z

...so2.carbon.email.mgt/src/main/java/org/wso2/carbon/email/mgt/constants/I18nMgtConstants.java

@@ -37,8 +37,8 @@ private I18nMgtConstants() {}
    public static final String SMS_CONF_DIRECTORY = "sms";
    public static final String EMAIL_ADMIN_CONF_FILE = "email-admin-config.xml";
    public static final String SMS_TEMPLAE_ADMIN_CONF_FILE = "sms-templates-admin-config.xml";
-    public static final String DEFAULT_EMAIL_LOCALE = "en_us";
-    public static final String DEFAULT_SMS_NOTIFICATION_LOCALE = "en_us";
+    public static final String DEFAULT_EMAIL_LOCALE = "en_US";


Will there be any impact in changing these constants?

This change no longer needed with new impl: #230

darshanasbg · 2024-04-08T21:53:41Z

Improved impl has been raised with #229 & #230. Hence, closing this PR.

Darshana Gunawardana and others added 22 commits March 25, 2024 21:08

Add DB based impl

6f71142

Improve DB Based EmailTemplatemanager

fd065e8

Improve error handling in Notification Scenario and Refactoring

76478ad

Improve error handling in Org Notification Templates

e41901d

Refactor Email Channel Constant

33c89b7

Improve error handling in App Notification Templates

e2b2ca5

Fix get notification scenario & update template flows

e7376c4

Fix app template update flow

fe70f24

Add DB based NotificationTemplateManager impl

d367e5e

Reformat new classes

9dd886d

Reformat existing classes

a232c2e

Refactor & fix unit tests

f7cfe9a

Refactor unused variable

bf53622

Add license

fb14cac

Refactor logs & improve default template add

2f240fb

Fix default template debug log

d2838fb

Fix todos & comments

e380f62

Refactor DBBasedEmailTemplateManager

42199a5

Fix todos & comments in DBBasedNotificationTemplateManager

f109ea5

Handle app id null flows

9f1fdbb

Refactor duplicate code & remove unused constants

2dc975e

Remove references to new db impl

0d06136

darshanasbg requested a review from Yoshani March 26, 2024 10:26

Yoshani reviewed Mar 28, 2024

View reviewed changes

darshanasbg added 5 commits March 28, 2024 23:40

Handle case sensitive locale names

a1d7c72

Refactor add & update methods

d68b1e6

Use jdbc named templates for notification scenario

68339e0

Use jdbc named templates for org notification templates

5acf66a

Use jdbc named templates for app notification templates

3ff0fca

darshanasbg mentioned this pull request Mar 28, 2024

Add Named JDBC Template support in Jdbc Utils in identity core wso2/carbon-identity-framework#5607

Merged

madurangasiriwardena reviewed Apr 1, 2024

View reviewed changes

darshanasbg closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DB Based Template Management Support #227

Add DB Based Template Management Support #227

darshanasbg commented Mar 26, 2024

Yoshani Mar 27, 2024

darshanasbg Apr 8, 2024

CLAassistant commented Mar 28, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

madurangasiriwardena Apr 1, 2024

darshanasbg Apr 8, 2024

darshanasbg commented Apr 8, 2024

		String[] whiteListPatterns = {TEMPLATE_REGEX_KEY};
		String[] blackListPatterns = {REGISTRY_INVALID_CHARS};

Add DB Based Template Management Support #227

Add DB Based Template Management Support #227

Conversation

darshanasbg commented Mar 26, 2024

Proposed changes in this pull request

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darshanasbg commented Apr 8, 2024