[DSC-193] tests added, code review fixes, other fixes in pagination and ordering

dspace-api/src/main/java/org/dspace/content/CommunityServiceImpl.java

@@ -219,15 +219,15 @@ public class CommunityServiceImpl extends DSpaceObjectServiceImpl<Community> imp
 
     @Override
     public Bitstream setLogo(Context context, Community community, InputStream is)
-        throws AuthorizeException, IOException, SQLException {
+            throws AuthorizeException, IOException, SQLException {
         // Check authorisation
         // authorized to remove the logo when DELETE rights
         // authorized when canEdit
         if (!((is == null) && authorizeService.authorizeActionBoolean(
-            context, community, Constants.DELETE))) {
+                context, community, Constants.DELETE))) {
             canEdit(context, community);
         }
-        subscribeService.deleteByDspaceObject(context, community);
+
         // First, delete any existing logo
         Bitstream oldLogo = community.getLogo();
         if (oldLogo != null) {
@@ -244,7 +244,7 @@ public class CommunityServiceImpl extends DSpaceObjectServiceImpl<Community> imp
             // now create policy for logo bitstream
             // to match our READ policy
             List<ResourcePolicy> policies = authorizeService
-                .getPoliciesActionFilter(context, community, Constants.READ);
+                    .getPoliciesActionFilter(context, community, Constants.READ);
             authorizeService.addPolicies(context, policies, newLogo);
 
             log.info(LogHelper.getHeader(context, "set_logo",

dspace-api/src/main/java/org/dspace/eperson/SubscribeCLITool.java

@@ -104,6 +104,9 @@ public class SubscribeCLITool {
 
         // Go through the list collating subscriptions for each e-person
         for (Subscription subscription : subscriptions) {
+            if (!(subscription.getdSpaceObject() != null && subscription.getdSpaceObject() instanceof Collection)) {
+                continue;
+            }
             // Does this row relate to the same e-person as the last?
             if ((currentEPerson == null)
                 || (!subscription.getePerson().getID().equals(currentEPerson

dspace-api/src/main/java/org/dspace/eperson/SubscribeServiceImpl.java

@@ -49,10 +49,10 @@ public class SubscribeServiceImpl implements SubscribeService {
     public List<Subscription> findAll(Context context, String resourceType,
                                       Integer limit, Integer offset) throws Exception {
         if (resourceType == null) {
-            return subscriptionDAO.findAllOrderedByEPerson(context);
+            return subscriptionDAO.findAllOrderedById(context, limit, offset);
         } else {
             if (resourceType.equals("Item") || resourceType.equals("Collection") || resourceType.equals("Community")) {
-                return subscriptionDAO.findAllOrderedByEPersonAndResourceType(context, resourceType, limit, offset);
+                return subscriptionDAO.findAllOrderedByIDAndResourceType(context, resourceType, limit, offset);
             } else {
                 log.error("Resource type must be Item, Collection or Community");
                 throw new Exception("Resource type must be Item, Collection or Community");
@@ -69,13 +69,13 @@ public class SubscribeServiceImpl implements SubscribeService {
         if (authorizeService.isAdmin(context)
                 || ((context.getCurrentUser() != null) && (context
                 .getCurrentUser().getID().equals(eperson.getID())))) {
-            Subscription new_subscription = subscriptionDAO.create(context, new Subscription());
+            Subscription newSubscription = subscriptionDAO.create(context, new Subscription());
             subscriptionParameterList.forEach(subscriptionParameter ->
-                    new_subscription.addParameter(subscriptionParameter));
+                    newSubscription.addParameter(subscriptionParameter));
-            new_subscription.setePerson(eperson);
+            newSubscription.setePerson(eperson);
-            new_subscription.setdSpaceObject(dSpaceObject);
+            newSubscription.setdSpaceObject(dSpaceObject);
-            new_subscription.setType(type);
+            newSubscription.setType(type);
-            return new_subscription;
+            return newSubscription;
         } else {
             throw new AuthorizeException(
                     "Only admin or e-person themselves can subscribe");
@@ -153,8 +153,13 @@ public class SubscribeServiceImpl implements SubscribeService {
     }
 
     @Override
-    public Subscription findById(Context context, int id) throws SQLException {
+    public Subscription findById(Context context, int id) throws SQLException, AuthorizeException {
-        return subscriptionDAO.findByID(context, Subscription.class, id);
+        Subscription subscription = subscriptionDAO.findByID(context, Subscription.class, id);
+        if (context.getCurrentUser().equals(subscription.getePerson()) ||
+                authorizeService.isAdmin(context, context.getCurrentUser())) {
+            return  subscription;
+        }
+        throw new AuthorizeException("Only admin or e-person themselves can edit the subscription");
     }
 
     @Override

dspace-api/src/main/java/org/dspace/eperson/dao/SubscriptionDAO.java

@@ -39,8 +39,8 @@ public interface SubscriptionDAO extends GenericDAO<Subscription> {
     public void deleteByDSOAndEPerson(Context context, DSpaceObject dSpaceObject, EPerson eperson)
             throws SQLException;
 
-    public List<Subscription> findAllOrderedByEPersonAndResourceType(Context context, String resourceType,
+    public List<Subscription> findAllOrderedByIDAndResourceType(Context context, String resourceType,
                                                       Integer limit, Integer offset) throws SQLException;
 
-    public List<Subscription> findAllOrderedByEPerson(Context context) throws SQLException;
+    public List<Subscription> findAllOrderedById(Context context, Integer limit, Integer offset) throws SQLException;
 }

dspace-api/src/main/java/org/dspace/eperson/dao/impl/SubscriptionDAOImpl.java

@@ -42,6 +42,9 @@ public class SubscriptionDAOImpl extends AbstractHibernateDAO<Subscription> impl
         Root<Subscription> subscriptionRoot = criteriaQuery.from(Subscription.class);
         criteriaQuery.select(subscriptionRoot);
         criteriaQuery.where(criteriaBuilder.equal(subscriptionRoot.get(Subscription_.ePerson), eperson));
+        List<javax.persistence.criteria.Order> orderList = new LinkedList<>();
+        orderList.add(criteriaBuilder.asc(subscriptionRoot.get(Subscription_.id)));
+        criteriaQuery.orderBy(orderList);
         return list(context, criteriaQuery, false, Subscription.class, limit, offset);
 
     }
@@ -61,6 +64,9 @@ public class SubscriptionDAOImpl extends AbstractHibernateDAO<Subscription> impl
                         subscriptionRoot.get(Subscription_.ePerson), eperson),
                 criteriaBuilder.equal(subscriptionRoot.get(Subscription_.dSpaceObject), dSpaceObject)
         ));
+        List<javax.persistence.criteria.Order> orderList = new LinkedList<>();
+        orderList.add(criteriaBuilder.asc(subscriptionRoot.get(Subscription_.id)));
+        criteriaQuery.orderBy(orderList);
         return list(context, criteriaQuery, false, Subscription.class, limit, offset);
     }
 
@@ -92,9 +98,10 @@ public class SubscriptionDAOImpl extends AbstractHibernateDAO<Subscription> impl
     }
 
     @Override
-    public List<Subscription> findAllOrderedByEPersonAndResourceType(Context context, String resourceType,
+    public List<Subscription> findAllOrderedByIDAndResourceType(Context context, String resourceType,
                                                       Integer limit, Integer offset) throws SQLException {
-        String hqlQuery = "select s from Subscription s join %s dso ON dso.id = s.dSpaceObject ORDER BY eperson_id";
+        String hqlQuery = "select s from Subscription s join %s dso " +
+                "ON dso.id = s.dSpaceObject ORDER BY subscription_id";
         if (resourceType != null) {
             hqlQuery = String.format(hqlQuery, resourceType);
         }
@@ -109,14 +116,14 @@ public class SubscriptionDAOImpl extends AbstractHibernateDAO<Subscription> impl
         return query.getResultList();
     }
     @Override
-    public List<Subscription> findAllOrderedByEPerson(Context context) throws SQLException {
+    public List<Subscription> findAllOrderedById(Context context, Integer limit, Integer offset) throws SQLException {
         CriteriaBuilder criteriaBuilder = getCriteriaBuilder(context);
         CriteriaQuery criteriaQuery = getCriteriaQuery(criteriaBuilder, Subscription.class);
         Root<Subscription> subscriptionRoot = criteriaQuery.from(Subscription.class);
         criteriaQuery.select(subscriptionRoot);
         List<javax.persistence.criteria.Order> orderList = new LinkedList<>();
-        orderList.add(criteriaBuilder.asc(subscriptionRoot.get(Subscription_.ePerson)));
+        orderList.add(criteriaBuilder.asc(subscriptionRoot.get(Subscription_.id)));
         criteriaQuery.orderBy(orderList);
-        return list(context, criteriaQuery, false, Subscription.class, -1, -1);
+        return list(context, criteriaQuery, false, Subscription.class, limit, offset);
     }
 }

dspace-api/src/main/java/org/dspace/eperson/service/SubscribeService.java

@@ -157,7 +157,7 @@ public interface SubscribeService {
      * @param id the id of subscription to be searched
      * @throws SQLException An exception that provides information on a database access error or other errors.
      */
-    public Subscription findById(Context context, int id) throws SQLException;
+    public Subscription findById(Context context, int id) throws SQLException, AuthorizeException;
 
     /**
      * Updates a subscription by id

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/SubscriptionDSpaceObjectLinkRepository.java

@@ -14,6 +14,7 @@ import javax.servlet.http.HttpServletRequest;
 import org.dspace.app.rest.model.DSpaceObjectRest;
 import org.dspace.app.rest.model.SubscriptionRest;
 import org.dspace.app.rest.projection.Projection;
+import org.dspace.authorize.AuthorizeException;
 import org.dspace.core.Context;
 import org.dspace.eperson.Subscription;
 import org.dspace.eperson.service.SubscribeService;
@@ -38,7 +39,7 @@ public class SubscriptionDSpaceObjectLinkRepository extends AbstractDSpaceRestRe
     public DSpaceObjectRest getDSpaceObject(@Nullable HttpServletRequest request,
                                             Integer subscriptionId,
                                             @Nullable Pageable optionalPageable,
-                                            Projection projection) {
+                                            Projection projection) throws AuthorizeException {
         try {
             Context context = obtainContext();
             Subscription subscription = subscribeService.findById(context, subscriptionId);
@@ -51,6 +52,8 @@ public class SubscriptionDSpaceObjectLinkRepository extends AbstractDSpaceRestRe
             return converter.toRest(initializer.getImplementation(), projection);
         } catch (SQLException e) {
             throw new RuntimeException(e);
+        } catch (AuthorizeException e) {
+            throw new AuthorizeException(e.getMessage());
         }
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/SubscriptionEPersonLinkRepository.java

@@ -14,6 +14,7 @@ import javax.servlet.http.HttpServletRequest;
 import org.dspace.app.rest.model.EPersonRest;
 import org.dspace.app.rest.model.SubscriptionRest;
 import org.dspace.app.rest.projection.Projection;
+import org.dspace.authorize.AuthorizeException;
 import org.dspace.core.Context;
 import org.dspace.eperson.Subscription;
 import org.dspace.eperson.service.SubscribeService;
@@ -34,7 +35,7 @@ public class SubscriptionEPersonLinkRepository extends AbstractDSpaceRestReposit
     public EPersonRest getEPerson(@Nullable HttpServletRequest request,
                                   Integer subscriptionId,
                                   @Nullable Pageable optionalPageable,
-                                  Projection projection) {
+                                  Projection projection) throws AuthorizeException {
         try {
             Context context = obtainContext();
             Subscription subscription = subscribeService.findById(context, subscriptionId);
@@ -46,5 +47,8 @@ public class SubscriptionEPersonLinkRepository extends AbstractDSpaceRestReposit
         } catch (SQLException e) {
             throw new RuntimeException(e);
         }
+        catch (AuthorizeException e) {
+            throw new AuthorizeException(e.getMessage());
+        }
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/SubscriptionRestRepository.java

@@ -83,6 +83,8 @@ public class SubscriptionRestRepository extends DSpaceRestRepository
             return converter.toRest(subscription, utils.obtainProjection());
         } catch (SQLException sqlException) {
             throw new RuntimeException(sqlException.getMessage(), sqlException);
+        } catch (AuthorizeException authorizeException) {
+            throw new RuntimeException(authorizeException.getMessage());
         }
     }
 
@@ -96,7 +98,7 @@ public class SubscriptionRestRepository extends DSpaceRestRepository
                     pageable.getPageSize(), Math.toIntExact(pageable.getOffset()));
             return converter.toRestPage(subscriptionList, pageable, utils.obtainProjection());
         } catch (Exception e) {
-            throw new RuntimeException(e.getMessage(), e);
+            throw new RuntimeException(e.getMessage());
         }
     }
 
@@ -205,6 +207,8 @@ public class SubscriptionRestRepository extends DSpaceRestRepository
             }
         } catch (SQLException e) {
             throw new ResourceNotFoundException(notFoundException);
+        } catch (AuthorizeException e) {
+            throw new AuthorizeException(e.getMessage());
         }
         if (id.equals(subscription.getID())) {
             List<SubscriptionParameter> subscriptionParameterList = new ArrayList<>();
@@ -218,6 +222,7 @@ public class SubscriptionRestRepository extends DSpaceRestRepository
             }
             subscription = subscribeService.updateSubscription(context, id, ePerson,
                     dSpaceObject, subscriptionParameterList, subscriptionRest.getSubscriptionType());
+            context.commit();
             return converter.toRest(subscription, utils.obtainProjection());
         } else {
             throw new IllegalArgumentException("The id in the Json and the id in the url do not match: "
@@ -240,6 +245,10 @@ public class SubscriptionRestRepository extends DSpaceRestRepository
             resourcePatch.patch(context, subscription, patch.getOperations());
         } catch (SQLException e) {
             throw new RuntimeException(e.getMessage(), e);
+        } catch (AuthorizeException authorizeException) {
+            throw new AuthorizeException(authorizeException.getMessage());
+        } catch (RuntimeException runtimeException) {
+            throw new RuntimeException(runtimeException.getMessage());
         }
     }
 

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/patch/operation/ResearcherProfileVisibleReplaceOperation.java

@@ -21,7 +21,7 @@ import org.springframework.stereotype.Component;
 
 /**
  * Implementation for ResearcherProfile visibility patches.
- *SubscriptionParameterAddOperationpatches
+ *
  * Example:
  * <code> curl -X PATCH http://${dspace.server.url}/api/eperson/profiles/<:id-eperson> -H "
  * Content-Type: application/json" -d '[{ "op": "replace", "path": "

dspace-server-webapp/src/test/java/org/dspace/app/rest/SubscriptionRestRepositoryIT.java

@@ -80,13 +80,10 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
         context.restoreAuthSystemState();
     }
 
+    // FIND ALL
     @Test
     public void findAll() throws Exception {
         context.turnOffAuthorisationSystem();
-        //When we call the root endpoint as anonymous user
-        getClient().perform(get("/api/core/subscriptions"))
-                //The status has to be 401 Not Authorized
-                .andExpect(status().isUnauthorized());
         String token = getAuthToken(admin.getEmail(), password);
         List<SubscriptionParameter> subscriptionParameterList = new ArrayList<>();
         SubscriptionParameter subscriptionParameter = new SubscriptionParameter();
@@ -118,15 +115,80 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
     }
 
     @Test
-    public void findByIdAsAdministrator() throws Exception {
+    public void findAllAnonymous() throws Exception {
+        //When we call the root endpoint as anonymous user
+        getClient().perform(get("/api/core/subscriptions"))
+                //The status has to be 401 Not Authorized
+                .andExpect(status().isUnauthorized());
+    }
+
+    @Test
+    public void findAllAsUser() throws Exception {
+        context.turnOffAuthorisationSystem();
+        String token = getAuthToken(eperson.getEmail(), password);
+        context.restoreAuthSystemState();
+        //When we call the root endpoint as simple user
+        getClient(token).perform(get("/api/core/subscriptions"))
+                //The status has to be 403 Forbidden
+                .andExpect(status().isForbidden());
+    }
+
+    @Test
+    public void findAllWithResourceType() throws Exception {
         context.turnOffAuthorisationSystem();
         //When we call the root endpoint as anonymous user
         getClient().perform(get("/api/core/subscriptions"))
-                //The status has to be 403 Not Authorized
+                //The status has to be 401 Not Authorized
                 .andExpect(status().isUnauthorized());
         String token = getAuthToken(admin.getEmail(), password);
         List<SubscriptionParameter> subscriptionParameterList = new ArrayList<>();
         SubscriptionParameter subscriptionParameter = new SubscriptionParameter();
+        subscriptionParameter.setName("Frequency");
+        subscriptionParameter.setValue("Daily");
+        subscriptionParameterList.add(subscriptionParameter);
+        Subscription subscription = SubscribeBuilder.subscribeBuilder(context, "TypeTest", publicItem, admin, subscriptionParameterList).build();
+        subscriptionParameter.setSubscription(subscription);
+        //When we call the root endpoint
+        context.restoreAuthSystemState();
+        getClient(token).perform(get("/api/core/subscriptions?resourceType=Item"))
+                //The status has to be 200 OK
+                .andExpect(status().isOk())
+                //We expect the content type to be "application/hal+json;charset=UTF-8"
+                .andExpect(content().contentType(contentType))
+                //By default we expect at least 1 submission forms so this to be reflected in the page object
+                .andExpect(jsonPath("$.page.size", is(20)))
+                .andExpect(jsonPath("$.page.totalElements", greaterThanOrEqualTo(1)))
+                .andExpect(jsonPath("$.page.totalPages", greaterThanOrEqualTo(1)))
+                .andExpect(jsonPath("$.page.number", is(0)))
+                .andExpect(jsonPath("$._embedded.subscriptions[0].subscriptionType", is("TypeTest")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0]._links.dSpaceObject.href", Matchers.startsWith(REST_SERVER_URL + "core/subscriptions")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0]._links.dSpaceObject.href", Matchers.endsWith("dSpaceObject")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0]._links.ePerson.href", Matchers.startsWith(REST_SERVER_URL + "core/subscriptions")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0]._links.ePerson.href", Matchers.endsWith("ePerson")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0].subscriptionParameterList[0].name", is("Frequency")))
+                .andExpect(jsonPath("$._embedded.subscriptions[0].subscriptionParameterList[0].value", is("Daily")))
+                .andExpect(jsonPath("$._links.self.href", Matchers.is(REST_SERVER_URL + "core/subscriptions?resourceType=Item")));
+        // search for subscriptions related with collections
+        getClient(token).perform(get("/api/core/subscriptions?resourceType=Collection"))
+                //The status has to be 200 OK
+                .andExpect(status().isOk())
+                //We expect the content type to be "application/hal+json;charset=UTF-8"
+                .andExpect(content().contentType(contentType))
+                //By default we expect at least 1 submission forms so this to be reflected in the page object
+                .andExpect(jsonPath("$.page.size", is(20)))
+                .andExpect(jsonPath("$.page.totalElements", greaterThanOrEqualTo(0)))
+                .andExpect(jsonPath("$.page.totalPages", greaterThanOrEqualTo(0)))
+                .andExpect(jsonPath("$.page.number", is(0)))
+                .andExpect(jsonPath("$._links.self.href", Matchers.is(REST_SERVER_URL + "core/subscriptions?resourceType=Collection")));
+    }
+
+    // FIND BY ID
+    @Test
+    public void findByIdAsAdministrator() throws Exception {
+        context.turnOffAuthorisationSystem();
+        String token = getAuthToken(admin.getEmail(), password);
+        List<SubscriptionParameter> subscriptionParameterList = new ArrayList<>();
+        SubscriptionParameter subscriptionParameter = new SubscriptionParameter();
         subscriptionParameter.setName("Parameter");
         subscriptionParameter.setValue("ValueParameter");
         subscriptionParameterList.add(subscriptionParameter);
@@ -167,6 +229,24 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
                 .andExpect(status().isUnauthorized());
     }
 
+    @Test
+    public void findByIdNotAsSubscriberNotAsAdmin() throws Exception {
+        context.turnOffAuthorisationSystem();
+        String token = getAuthToken(eperson.getEmail(), password);
+        List<SubscriptionParameter> subscriptionParameterList = new ArrayList<>();
+        SubscriptionParameter subscriptionParameter = new SubscriptionParameter();
+        subscriptionParameter.setName("Parameter");
+        subscriptionParameter.setValue("ValueParameter");
+        subscriptionParameterList.add(subscriptionParameter);
+        Subscription subscription = SubscribeBuilder.subscribeBuilder(context, "TestType", publicItem, admin, subscriptionParameterList).build();
+        context.restoreAuthSystemState();
+        //When we call the root endpoint
+        getClient(token).perform(get("/api/core/subscriptions/" + subscription.getID()))
+                //The status has to be 500
+                .andExpect(status().isInternalServerError());
+    }
+
+    // FIND ALL BY EPERSON/DSO
     @Test
     public void findAllSubscriptionsByEPerson() throws Exception {
         //When we call the root endpoint as anonymous user
@@ -228,6 +308,7 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
                 .andExpect(status().isUnauthorized());
     }
 
+    // ADD
     @Test
     public void addSubscriptionNotLoggedIn() throws Exception {
         SubscriptionParameterRest subscriptionParameterRest = new SubscriptionParameterRest();
@@ -287,6 +368,7 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
                 .andExpect(jsonPath("$._links.ePerson.href", Matchers.endsWith("ePerson")));
     }
 
+    // PUT
     @Test
     public void editSubscriptionAnonymous() throws Exception {
         context.turnOffAuthorisationSystem();
@@ -330,12 +412,22 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
         subscriptionParameterList.add(subscriptionParameter);
         Subscription subscription = SubscribeBuilder.subscribeBuilder(context, "TestType", publicItem, eperson, subscriptionParameterList).build();
         context.restoreAuthSystemState();
+        ObjectMapper objectMapper = new ObjectMapper();
+        String token = getAuthToken(epersonIT.getEmail(), password);
+        Map<String, Object> newSubscription = new HashMap<>();
+        newSubscription.put("type", "test");
+        List<Map<String, Object>> list = new ArrayList<>();
+        Map<String, Object> sub_list = new HashMap<>();
+        sub_list.put("name", "frequency");
+        sub_list.put("value", "daily");
+        list.add(sub_list);
+        newSubscription.put("subscriptionParameterList", list);
         //When we call the root endpoint as anonymous user
-        getClient().perform(put("/api/core/subscriptions/" + subscription.getID() + "?dspace_object_id=" + publicItem.getID() + "&eperson_id=" + admin.getID())
+        getClient(token).perform(put("/api/core/subscriptions/" + subscription.getID() + "?dspace_object_id=" + publicItem.getID() + "&eperson_id=" + admin.getID())
                         .content(objectMapper.writeValueAsString(newSubscription))
                         .contentType(MediaType.APPLICATION_JSON_VALUE))
-                //The status has to be 403 Not Authorized
+                //The status has to be 500 Error
-                .andExpect(status().isUnauthorized());
+                .andExpect(status().isInternalServerError());
     }
 
     @Test
@@ -378,6 +470,7 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
                 .andExpect(jsonPath("$._links.ePerson.href", Matchers.endsWith("/ePerson")));
     }
 
+    // DELETE
     @Test
     public void deleteSubscriptionNotAsSubscriberNotAsAdmin() throws Exception {
         context.turnOffAuthorisationSystem();
@@ -416,6 +509,7 @@ public class SubscriptionRestRepositoryIT extends AbstractControllerIntegrationT
                 .andExpect(status().isOk());
     }
 
+    // PATCH
     @Test
     public void patchReplaceSubscriptionParameterAsAdmin() throws Exception {
         context.turnOffAuthorisationSystem();