Minor refactoring - Design level refactors (#2781)

* Minor refactoring - Replace conditional with polymorphism. * Minor refactoring - Move Field - worked on comment - // These 2 should be (public) constants on their respective service classes, not here * Minor refactoring - Rename Field - making the code more readable * Minor refactoring - Rename Field - making the code more readable * Minor refactoring - Move method - the method getUserLayoutTuple seems more relevant to IUserLayoutStore. Hence moving it * Minor refactoring - Extract class PortalaConstants . Identified a cohesive set of variables/constants and grouped them into their own class. * formatting * Refactoring - Extract class - Extracted event coordination helper methods from the PortletEventCoordinatationService into a new class named PortletEventCoordinationHelper. * Refactoring - Extract class - Extracted event coordination helper methods from the PortletEventCoordinatationService into a new class named PortletEventCoordinationHelper. --------- Co-authored-by: himanshi <[email protected]>
uPortal-Project · Aug 27, 2024 · 4364fd1 · 4364fd1
1 parent 6fad6aa
commit 4364fd1
Show file tree

Hide file tree

Showing 15 changed files with 447 additions and 384 deletions.
diff --git a/uPortal-api/uPortal-api-rest/src/main/java/org/apereo/portal/rest/PagsRESTController.java b/uPortal-api/uPortal-api-rest/src/main/java/org/apereo/portal/rest/PagsRESTController.java
@@ -75,17 +75,17 @@ public class PagsRESTController {
          * This step is necessary;  the incoming URLs will sometimes have '+'
          * characters for spaces, and the @PathVariable magic doesn't convert them.
          */
-        String name;
+        String decodedPagsGroupName;
         try {
-            name = URLDecoder.decode(pagsGroupName, "UTF-8");
+            decodedPagsGroupName = URLDecoder.decode(pagsGroupName, "UTF-8");
         } catch (UnsupportedEncodingException e) {
             res.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
             return "{ 'error': '" + e.toString() + "' }";
         }
 
         IPerson person = personManager.getPerson(request);
         IPersonAttributesGroupDefinition pagsGroup =
-                this.pagsService.getPagsDefinitionByName(person, name);
+                this.pagsService.getPagsDefinitionByName(person, decodedPagsGroupName);
         return respondPagsGroupJson(res, pagsGroup, person, HttpServletResponse.SC_FOUND);
     }
 
@@ -106,9 +106,9 @@ public class PagsRESTController {
          * This step is necessary;  the incoming URLs will sometimes have '+'
          * characters for spaces, and the @PathVariable magic doesn't convert them.
          */
-        String name;
+        String decodedParentGroupName;
         try {
-            name = URLDecoder.decode(parentGroupName, "UTF-8");
+            decodedParentGroupName = URLDecoder.decode(parentGroupName, "UTF-8");
         } catch (UnsupportedEncodingException e) {
             res.setStatus(HttpServletResponse.SC_BAD_REQUEST);
             return "{ 'error': '" + e.getMessage() + "' }";
@@ -125,10 +125,12 @@ public class PagsRESTController {
         // Obtain a real reference to the parent group
         EntityIdentifier[] eids =
                 GroupService.searchForGroups(
-                        name, IGroupConstants.SearchMethod.DISCRETE, IPerson.class);
+                        decodedParentGroupName,
+                        IGroupConstants.SearchMethod.DISCRETE,
+                        IPerson.class);
         if (eids.length == 0) {
             res.setStatus(HttpServletResponse.SC_BAD_REQUEST);
-            return "{ 'error': 'Parent group does not exist: " + name + "' }";
+            return "{ 'error': 'Parent group does not exist: " + decodedParentGroupName + "' }";
         }
         IEntityGroup parentGroup =
                 (IEntityGroup) GroupService.getGroupMember(eids[0]); // Names must be unique

diff --git a/...ags/src/main/java/org/apereo/portal/groups/pags/dao/EntityPersonAttributesGroupStore.java b/...ags/src/main/java/org/apereo/portal/groups/pags/dao/EntityPersonAttributesGroupStore.java
@@ -38,6 +38,7 @@
 import org.apereo.portal.groups.pags.IPersonTester;
 import org.apereo.portal.groups.pags.PagsGroup;
 import org.apereo.portal.groups.pags.TestGroup;
+import org.apereo.portal.groups.pags.dao.jpa.PagsGroupService;
 import org.apereo.portal.security.IPerson;
 import org.apereo.portal.security.PersonFactory;
 import org.apereo.portal.security.provider.RestrictedPerson;
@@ -111,7 +112,7 @@ public boolean contains(IEntityGroup group, IGroupMember member) {
         if (member.isGroup()) {
             // PAGS groups may only contain other PAGS groups (and people, of course)
             final IEntityGroup ieg = (IEntityGroup) member;
-            if (!PagsService.SERVICE_NAME_PAGS.equals(ieg.getServiceName().toString())) {
+            if (!PagsGroupService.SERVICE_NAME_PAGS.equals(ieg.getServiceName().toString())) {
                 return false;
             }
         }
@@ -233,7 +234,7 @@ public Iterator<IEntityGroup> findParentGroups(IGroupMember member) throws Group
         if (member.isGroup()) {
             // PAGS groups may only contain other PAGS groups (and people, of course)
             final IEntityGroup ieg = (IEntityGroup) member;
-            if (PagsService.SERVICE_NAME_PAGS.equals(ieg.getServiceName().toString())) {
+            if (PagsGroupService.SERVICE_NAME_PAGS.equals(ieg.getServiceName().toString())) {
                 result = findParentGroupsForGroup((IEntityGroup) member);
             }
         } else {

diff --git a/...oups/uPortal-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/PagsService.java b/...oups/uPortal-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/PagsService.java
@@ -14,13 +14,18 @@
  */
 package org.apereo.portal.groups.pags.dao;
 
+import static org.apereo.portal.groups.pags.dao.jpa.LocalGroupService.SERVICE_NAME_LOCAL;
+import static org.apereo.portal.groups.pags.dao.jpa.PagsGroupService.SERVICE_NAME_PAGS;
+
 import java.util.HashSet;
 import java.util.Set;
 import java.util.regex.Pattern;
 import org.apache.commons.lang3.StringUtils;
 import org.apereo.portal.EntityIdentifier;
 import org.apereo.portal.groups.IEntityGroup;
 import org.apereo.portal.groups.IGroupConstants;
+import org.apereo.portal.groups.pags.dao.jpa.LocalGroupService;
+import org.apereo.portal.groups.pags.dao.jpa.PagsGroupService;
 import org.apereo.portal.portlet.om.IPortletDefinition;
 import org.apereo.portal.security.IAuthorizationPrincipal;
 import org.apereo.portal.security.IPermission;
@@ -42,9 +47,8 @@
 @Service
 public final class PagsService {
 
-    // These 2 should be (public) constants on their respective service classes, not here
-    private static final String SERVICE_NAME_LOCAL = "local";
-    public static final String SERVICE_NAME_PAGS = "pags";
+    @Autowired private PagsGroupService pagsGroupService;
+    @Autowired private LocalGroupService localGroupService;
 
     private static final String GROUP_NAME_VALIDATOR_REGEX = "^[\\w ]{5,500}$"; // 5-500 characters
     private static final Pattern GROUP_NAME_VALIDATOR_PATTERN =
@@ -152,29 +156,15 @@ public IPersonAttributesGroupDefinition createPagsDefinition(
         IPersonAttributesGroupDefinition result =
                 pagsGroupDefDao.createPersonAttributesGroupDefinition(groupName, description);
         if (parent != null) {
-            // Should refactor this switch to instead choose a service and invoke a method on it
+
             switch (parent.getServiceName().toString()) {
                 case SERVICE_NAME_LOCAL:
-                    IEntityGroup member =
-                            GroupService.findGroup(
-                                    result.getCompositeEntityIdentifierForGroup().getKey());
-                    if (member == null) {
-                        String msg =
-                                "The specified group was created, but is not present in the store:  "
-                                        + result.getName();
-                        throw new RuntimeException(msg);
-                    }
-                    parent.addChild(member);
-                    parent.updateMembers();
+                    localGroupService.addMember(parent, result);
                     break;
                 case SERVICE_NAME_PAGS:
                     IPersonAttributesGroupDefinition parentDef =
                             getPagsGroupDefByName(parent.getName());
-                    Set<IPersonAttributesGroupDefinition> members =
-                            new HashSet<>(parentDef.getMembers());
-                    members.add(result);
-                    parentDef.setMembers(members);
-                    pagsGroupDefDao.updatePersonAttributesGroupDefinition(parentDef);
+                    pagsGroupService.addMember(parentDef, result);
                     break;
                 default:
                     String msg =

diff --git a/...al-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/jpa/LocalGroupService.java b/...al-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/jpa/LocalGroupService.java
@@ -0,0 +1,24 @@
+package org.apereo.portal.groups.pags.dao.jpa;
+
+import org.apereo.portal.groups.IEntityGroup;
+import org.apereo.portal.groups.pags.dao.IPersonAttributesGroupDefinition;
+import org.springframework.stereotype.Service;
+
+@Service
+public class LocalGroupService {
+    public static final String SERVICE_NAME_LOCAL = "local";
+
+    public void addMember(IEntityGroup parent, IPersonAttributesGroupDefinition result) {
+        IEntityGroup member =
+                org.apereo.portal.services.GroupService.findGroup(
+                        result.getCompositeEntityIdentifierForGroup().getKey());
+        if (member == null) {
+            String msg =
+                    "The specified group was created, but is not present in the store:  "
+                            + result.getName();
+            throw new RuntimeException(msg);
+        }
+        parent.addChild(member);
+        parent.updateMembers();
+    }
+}
diff --git a/...tal-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/jpa/PagsGroupService.java b/...tal-groups-pags/src/main/java/org/apereo/portal/groups/pags/dao/jpa/PagsGroupService.java
@@ -0,0 +1,26 @@
+package org.apereo.portal.groups.pags.dao.jpa;
+
+import java.util.HashSet;
+import java.util.Set;
+import org.apereo.portal.groups.pags.dao.IPersonAttributesGroupDefinition;
+import org.apereo.portal.groups.pags.dao.IPersonAttributesGroupDefinitionDao;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+@Service
+public class PagsGroupService {
+    public static final String SERVICE_NAME_PAGS = "pags";
+    @Autowired private IPersonAttributesGroupDefinitionDao pagsGroupDefDao;
+    private final Logger logger = LoggerFactory.getLogger(getClass());
+
+    public void addMember(
+            IPersonAttributesGroupDefinition parentDef, IPersonAttributesGroupDefinition result) {
+
+        Set<IPersonAttributesGroupDefinition> members = new HashSet<>(parentDef.getMembers());
+        members.add(result);
+        parentDef.setMembers(members);
+        pagsGroupDefDao.updatePersonAttributesGroupDefinition(parentDef);
+    }
+}
diff --git a/.../main/java/org/apereo/portal/groups/pags/dao/jpa/PersonAttributesGroupDefinitionImpl.java b/.../main/java/org/apereo/portal/groups/pags/dao/jpa/PersonAttributesGroupDefinitionImpl.java
@@ -41,7 +41,6 @@
 import org.apereo.portal.EntityIdentifier;
 import org.apereo.portal.groups.pags.dao.IPersonAttributesGroupDefinition;
 import org.apereo.portal.groups.pags.dao.IPersonAttributesGroupTestGroupDefinition;
-import org.apereo.portal.groups.pags.dao.PagsService;
 import org.apereo.portal.security.IPerson;
 import org.dom4j.DocumentHelper;
 import org.dom4j.QName;
@@ -135,7 +134,7 @@ public long getId() {
     @JsonIgnore
     public EntityIdentifier getCompositeEntityIdentifierForGroup() {
         return new EntityIdentifier(
-                PagsService.SERVICE_NAME_PAGS + "." + this.getName(), IPerson.class);
+                PagsGroupService.SERVICE_NAME_PAGS + "." + this.getName(), IPerson.class);
     }
 
     @Override

diff --git a/...l-layout/uPortal-layout-core/src/main/java/org/apereo/portal/layout/IUserLayoutStore.java b/...l-layout/uPortal-layout-core/src/main/java/org/apereo/portal/layout/IUserLayoutStore.java
@@ -199,4 +199,19 @@ void setUserLayout(
      * with greater precedence come before those with lower precedence.
      */
     double getFragmentPrecedence(long id);
+
+    /**
+     * Provides a {@link Tuple} containing the &quot;fragmentized&quot; version of a DLM fragment
+     * owner's layout, together with the username. This version of the layout consistent with what
+     * DLM uses internally for fragments, and is created by FragmentActivator.fragmentizeLayout.
+     * It's important that the version returned by this method matches what DLM uses internally
+     * because it will be used to establish relationships between fragment layout nodes and user
+     * customizations of DLM fragments.
+     *
+     * @param userName The username of the user for whom the layout is retrieved.
+     * @param userId The unique identifier of the user.
+     * @return A {@link Tuple} containing the username and the "fragmentized" version of the DLM
+     *     fragment owner's layout.
+     */
+    Tuple<String, DistributedUserLayout> getUserLayoutTuple(String userName, int userId);
 }
diff --git a/.../uPortal-layout-impl/src/main/java/org/apereo/portal/layout/dlm/NodeReferenceFactory.java b/.../uPortal-layout-impl/src/main/java/org/apereo/portal/layout/dlm/NodeReferenceFactory.java
@@ -22,11 +22,7 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apereo.portal.IUserIdentityStore;
-import org.apereo.portal.IUserProfile;
-import org.apereo.portal.UserProfile;
 import org.apereo.portal.layout.IUserLayoutStore;
-import org.apereo.portal.security.provider.BrokenSecurityContext;
-import org.apereo.portal.security.provider.PersonImpl;
 import org.apereo.portal.utils.Tuple;
 import org.apereo.portal.xml.XmlUtilities;
 import org.apereo.portal.xml.xpath.XPathOperations;
@@ -47,7 +43,7 @@ public final class NodeReferenceFactory {
     private static final Pattern DLM_PATH_REF_DELIM = Pattern.compile("\\:");
     private static final Pattern USER_NODE_PATTERN = Pattern.compile("\\A([a-zA-Z]\\d*)\\z");
     private static final Pattern DLM_NODE_PATTERN = Pattern.compile("u(\\d+)l\\d+([ns]\\d+)");
-
+    private static final int LAYOUT_ID_FIRST_NODE = 1;
     private final Log log = LogFactory.getLog(getClass());
 
     @Autowired private IUserLayoutStore layoutStore;
@@ -142,7 +138,7 @@ public Noderef getNoderefFromPathref(
         }
 
         final Tuple<String, DistributedUserLayout> userLayoutInfo =
-                getUserLayoutTuple(layoutOwnerName, layoutOwnerUserId);
+                layoutStore.getUserLayoutTuple(layoutOwnerName, layoutOwnerUserId);
         final Document userLayout = userLayoutInfo.second.getLayout();
 
         final Node targetNode =
@@ -178,21 +174,14 @@ public Noderef getNoderefFromPathref(
             final String structId = structIdAttr.getTextContent();
 
             if (isStructRef) {
-                return new Noderef(
-                        layoutOwnerUserId,
-                        1 /* TODO:  remove hard-coded layoutId=1 */,
-                        "s" + structId);
+                return new Noderef(layoutOwnerUserId, LAYOUT_ID_FIRST_NODE, "s" + structId);
             }
 
-            return new Noderef(
-                    layoutOwnerUserId, 1 /* TODO:  remove hard-coded layoutId=1 */, "n" + structId);
+            return new Noderef(layoutOwnerUserId, LAYOUT_ID_FIRST_NODE, "n" + structId);
         }
 
         final Node idAttr = attributes.getNamedItem("ID");
-        return new Noderef(
-                layoutOwnerUserId,
-                1 /* TODO:  remove hard-coded layoutId=1 */,
-                idAttr.getTextContent());
+        return new Noderef(layoutOwnerUserId, LAYOUT_ID_FIRST_NODE, idAttr.getTextContent());
     }
 
     /**
@@ -229,7 +218,7 @@ public Pathref getPathrefFromNoderef(
 
             final String userName = this.userIdentityStore.getPortalUserName(userId);
             final Tuple<String, DistributedUserLayout> userLayoutInfo =
-                    getUserLayoutTuple(userName, userId);
+                    layoutStore.getUserLayoutTuple(userName, userId);
 
             if (userLayoutInfo.second == null) {
                 this.log.warn(
@@ -291,35 +280,4 @@ public Pathref getPathrefFromNoderef(
 
         return result;
     }
-
-    /*
-     * Implementation.
-     */
-
-    /**
-     * Provides a {@link Tuple} containing the &quot;fragmentized&quot; version of a DLM fragment
-     * owner's layout, together with the username. This version of the layout consistent with what
-     * DLM uses internally for fragments, and is created by FragmentActivator.fragmentizeLayout.
-     * It's important that the version returned by this method matches what DLM uses internally
-     * because it will be used to establish relationships between fragment layout nodes and user
-     * customizations of DLM fragments.
-     *
-     * @param userId
-     * @return
-     */
-    /* TODO:  make private */ Tuple<String, DistributedUserLayout> getUserLayoutTuple(
-            String userName, int userId) {
-
-        final PersonImpl person = new PersonImpl();
-        person.setUserName(userName);
-        person.setID(userId);
-        person.setSecurityContext(new BrokenSecurityContext());
-
-        final IUserProfile profile =
-                layoutStore.getUserProfileByFname(person, UserProfile.DEFAULT_PROFILE_FNAME);
-        final DistributedUserLayout userLayout =
-                layoutStore.getUserLayout(person, (UserProfile) profile);
-
-        return new Tuple<String, DistributedUserLayout>(userName, userLayout);
-    }
 }
diff --git a/...al-layout-impl/src/main/java/org/apereo/portal/layout/dlm/RDBMDistributedLayoutStore.java b/...al-layout-impl/src/main/java/org/apereo/portal/layout/dlm/RDBMDistributedLayoutStore.java
@@ -33,10 +33,7 @@
 import java.util.regex.Pattern;
 import net.sf.ehcache.Ehcache;
 import org.apache.commons.lang.StringUtils;
-import org.apereo.portal.AuthorizationException;
-import org.apereo.portal.IUserIdentityStore;
-import org.apereo.portal.IUserProfile;
-import org.apereo.portal.PortalException;
+import org.apereo.portal.*;
 import org.apereo.portal.io.xml.IPortalDataHandlerService;
 import org.apereo.portal.jdbc.RDBMServices;
 import org.apereo.portal.layout.LayoutStructure;
@@ -1873,6 +1870,33 @@ private interface FormOfLayoutCorruption {
         String getMessage();
     }
 
+    /**
+     * Provides a {@link Tuple} containing the &quot;fragmentized&quot; version of a DLM fragment
+     * owner's layout, together with the username. This version of the layout consistent with what
+     * DLM uses internally for fragments, and is created by FragmentActivator.fragmentizeLayout.
+     * It's important that the version returned by this method matches what DLM uses internally
+     * because it will be used to establish relationships between fragment layout nodes and user
+     * customizations of DLM fragments.
+     *
+     * @param userName The username of the user for whom the layout is retrieved.
+     * @param userId The unique identifier of the user.
+     * @return A {@link Tuple} containing the username and the "fragmentized" version of the DLM
+     *     fragment owner's layout.
+     */
+    @Override
+    public Tuple<String, DistributedUserLayout> getUserLayoutTuple(String userName, int userId) {
+        final PersonImpl person = new PersonImpl();
+        person.setUserName(userName);
+        person.setID(userId);
+        person.setSecurityContext(new BrokenSecurityContext());
+
+        final IUserProfile profile =
+                this.getUserProfileByFname(person, UserProfile.DEFAULT_PROFILE_FNAME);
+        final DistributedUserLayout userLayout = this.getUserLayout(person, profile);
+
+        return new Tuple<>(userName, userLayout);
+    }
+
     private static final List<FormOfLayoutCorruption> KNOWN_FORMS_OF_LAYOUT_CORRUPTION =
             Collections.unmodifiableList(
                     Arrays.asList(