-
Notifications
You must be signed in to change notification settings - Fork 272
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
Minor refactoring - Design level refactors #2781
base: master
Are you sure you want to change the base?
Conversation
…d be (public) constants on their respective service classes, not here
… more relevant to IUserLayoutStore. Hence moving it
…esive set of variables/constants and grouped them into their own class.
…hods from the PortletEventCoordinatationService into a new class named PortletEventCoordinationHelper.
…hods from the PortletEventCoordinatationService into a new class named PortletEventCoordinationHelper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HimanshiVerma05!
The code looks good from reading through it.
My caution comes from this touching the PAGS and Layout subsystems, which aren't as heavily tested as I'd like while also being rather complex subsystems. Those still require some manual testing.
I've CC'ed other maintainers are reviewers to see if anyone could run some manual tests.
Some general tips:
- New tests are the quickest type of change to review
- PRs that adjust store are easier to review when they focus on one or a few subsystems, and/or a single type of refactor at a time.
- Features are also welcome, those are often mentioned/discussed on the mail list before they are added https://groups.google.com/a/apereo.org/g/uportal-dev
Checklist
Description of change
Refactored the code and worked on the comments-
"// Should refactor this switch to instead choose a service and invoke a method on it"
" // These 2 should be (public) constants on their respective service classes, not here"
in PagsService.java .