Optimize template addition & update by cross checking the content with system default templates #257
Conversation
|
PR builder started |
...on.email.mgt/src/main/java/org/wso2/carbon/email/mgt/store/InMemoryBasedTemplateManager.java
Outdated
Show resolved
Hide resolved
|
PR builder completed |
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10660876790
darshanasbg
force-pushed
the
OptimizeSystemTemplateManagement
branch
from
aedcee2 to
af62d1e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #257 +/- ##
============================================
+ Coverage 18.02% 19.30% +1.28%
- Complexity 170 181 +11
============================================
Files 53 52 -1
Lines 2869 2833 -36
Branches 323 323
============================================
+ Hits 517 547 +30
+ Misses 2309 2242 -67
- Partials 43 44 +1 ☔ View full report in Codecov by Sentry. |
darshanasbg
force-pushed
the
OptimizeSystemTemplateManagement
branch
from
b6ac39c to
c0884ff
Compare
darshanasbg
force-pushed
the
OptimizeSystemTemplateManagement
branch
2 times, most recently
from
e2f9e2e to
35b4d7a
Compare
darshanasbg
force-pushed
the
OptimizeSystemTemplateManagement
branch
from
35b4d7a to
d1dcf5b
Compare
|
PR builder started |
| @@ -39,7 +39,7 @@ | |||
| * This class have support for get, list, exists operations and any invocations to modification operations | |||
| * throws {@link UnsupportedOperationException}. | |||
| * This class expected to be use only with conjunction with another {@link TemplatePersistenceManager} implementation | |||
| * which supports full CRUD operations, hence {@link DefaultTemplateManager} provides that aggregation using this as a | |||
| * which supports full CRUD operations, hence {@link UnifiedTemplateManager} provides that aggregation using this as a | |||
| * fallback provider. | |||
| */ | |||
| public class InMemoryBasedTemplateManager implements TemplatePersistenceManager { | |||
There was a problem hiding this comment.
Renaming this to SystemDefaultTemplateManager would make much sense IMO.
There was a problem hiding this comment.
Agreed.. I also thought InMemoryBasedTemplateManager name only gives the unnecessary implementation detail yet not describe the duty of this class.. Will fix this.
There was a problem hiding this comment.
Fixed with 8cf3bf4
|
PR builder completed |
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/10696508535
Proposed changes in this pull request
$subject. Improves #256 along with renaming,
DefaultTemplateManagertoUnifiedTemplateManagerInMemoryBasedTemplateManagertoSystemDefaultTemplateManagerRelated to,
Main logical change
Main logical change of this PR is focused to the
UnifiedTemplateManager.addOrUpdateNotificationTemplate()method.This implementation changed in a way that, if the org template that is going to be added have same content as a system default template, don't proceed with persisting in a persistent storage, since its redundant. There is no change on the persistent logic for application templates.
Note that, above change to
UnifiedTemplateManager.addOrUpdateNotificationTemplate()method only a implementation change and does not change the behavior for any of its outside clients.Expected behavior for external clients:
For a given scenario lets denote,
addOrUpdateNotificationTemplate()param as c1ie: c1=c0, c2=null
Expected result: After completing the operation, get template returns c0(=c1).
ie: c1!=c0, c2=null
Expected result: After completing the operation, get template returns c1.
ie: c1=c0, c2!=null
Expected result: Before the update get template returned c2, after the update get template returns c0(=c1).
ie: c1!=c0, c2!=null
Expected result: Before the update get template returned c2, after the update get template returns c1.
This behavior kept same before and after this PR.
Actual change
ie: c1=c0, c2=null
Previous impl: c1 stored in the persistent storage which makes c2=c1(=c0)
New impl: skipping storing c1 which keeps c2=null
ie: c1!=c0, c2=null
Previous impl: c1 stored in the persistent storage which makes c2=c1
New impl: (no change) c1 stored in the persistent storage which makes c2=c1
ie: c1=c0, c2!=null
Previous impl: c2 updated in the persistent storage to be c1 which makes c2=c1(=c0)
New impl: c2 is deleted in the persistent storage which makes c2=null
Expected result: Before the update get template returned c2, after the update get template returns c0(=c1).
ie: c1!=c0, c2!=null
Previous impl: c2 updated in the persistent storage to be c1 which makes c2=c1
New impl: (no change) c2 updated in the persistent storage to be c1 which makes c2=c1
When should this PR be merged
Immediate.
Follow up actions
N/A