Minor refactoring - Design level refactors #2781
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.
HimanshiVerma05
changed the title
…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.
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
There was a problem hiding this comment.
The proper thing to do is reject this PR. There are too many refactors that cross concepts. There are changes to the group services, DLM. portal URL, portlet events, and a few more. The title of the PR also lacks description.
That all said, I have reviewed all the code changes and find that they improve the codebase. Nicely done, but please limit PRs to individual services unless you are working on code that covers interaction between services.
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 .