Skip to content

TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl#6006

Open
ShrutiSocrates wants to merge 1 commit intoopenmrs:masterfrom
ShrutiSocrates:TRUNK-fix-globalPropertyDeleted-implementation
Open

TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl#6006
ShrutiSocrates wants to merge 1 commit intoopenmrs:masterfrom
ShrutiSocrates:TRUNK-fix-globalPropertyDeleted-implementation

Conversation

@ShrutiSocrates
Copy link
Copy Markdown

Implements globalPropertyDeleted() to reset presentationLocales when LOCALE_ALLOWED_LIST
property is deleted, consistent with globalPropertyChanged() behavior. Added unit test to verify the behavior.

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-6605

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style
  • I have added tests to cover my changes
  • I ran mvn clean package
  • All new and existing tests passed
  • My pull request is based on latest master

Implements globalPropertyDeleted() to reset
presentationLocales when LOCALE_ALLOWED_LIST
property is deleted, consistent with
globalPropertyChanged() behavior.
@sonarqubecloud
Copy link
Copy Markdown

@0abhi007
Copy link
Copy Markdown

Good implementation! SonarCloud flagged 2 new issues on this PR, worth looking into before merge to keep the quality gate fully clean. Also, consider adding a test for when the deleted property key is NOT LOCALE_ALLOWED_LIST, to confirm there are no unintended side effects on other properties.

Comment on lines -700 to -701
// TODO Auto-generated method stub

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Kindly why did you remove line?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Those lines were auto-generated placeholder stubs with no actual implementation. Since I've now provided the real implementation for globalPropertyDeleted, the TODO stub was no longer needed and I removed it to keep the code clean.

Copy link
Copy Markdown
Contributor

@jwnasambu jwnasambu left a comment

Choose a reason for hiding this comment

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

Kindly is your ticket Id supposed to read something like this TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl and not TRUNK-6605: Implement globalPropertyDeleted to reset presentationLocales

@ShrutiSocrates
Copy link
Copy Markdown
Author

Kindly is your ticket Id supposed to read something like this TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl and not TRUNK-6605: Implement globalPropertyDeleted to reset presentationLocales

You're right, thank you for pointing that out! The commit message should reflect the broader implementation scope. I'll update it to TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl.

@ShrutiSocrates
Copy link
Copy Markdown
Author

Good implementation! SonarCloud flagged 2 new issues on this PR, worth looking into before merge to keep the quality gate fully clean. Also, consider adding a test for when the deleted property key is NOT LOCALE_ALLOWED_LIST, to confirm there are no unintended side effects on other properties.

Thanks for the feedback! I'll look into the 2 SonarCloud issues and address them before merge. I'll also add a test case to verify that deleting a property key other than LOCALE_ALLOWED_LIST doesn't cause any unintended side effects on other properties.

@ShrutiSocrates ShrutiSocrates changed the title TRUNK-6605: Implement globalPropertyDeleted to reset presentationLocales TRUNK-6605: Implement globalPropertyDeleted in AdministrationServiceImpl Mar 31, 2026
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