Bug 4104: REST API, update tag: use old values for update when parameters are missing

java/com.sap.sailing.server.gateway/src/com/sap/sailing/server/gateway/jaxrs/api/TagsResource.java

@@ -30,8 +30,9 @@ import com.sap.sse.common.TimePoint;
 import com.sap.sse.common.Util;
 import com.sap.sse.common.impl.MillisecondsTimePoint;
 
-@Path("/v1/{" + RaceLogServletConstants.PARAMS_LEADERBOARD_NAME + "}/{" + RaceLogServletConstants.PARAMS_RACE_COLUMN_NAME
-        + "}/{" + RaceLogServletConstants.PARAMS_RACE_FLEET_NAME + "}/tags")
+@Path("/v1/{" + RaceLogServletConstants.PARAMS_LEADERBOARD_NAME + "}/{"
+        + RaceLogServletConstants.PARAMS_RACE_COLUMN_NAME + "}/{" + RaceLogServletConstants.PARAMS_RACE_FLEET_NAME
+        + "}/tags")
 public class TagsResource extends AbstractSailingServerResource {
 
     private static final Logger logger = Logger.getLogger(TagsResource.class.getName());
@@ -144,12 +145,12 @@ public class TagsResource extends AbstractSailingServerResource {
     }
 
     /**
-     * Updates tag.
+     * Updates tag. When optional parameters <code>tag</code>, <code>comment</code>, <code>image</code> or
+     * <code>public</code> are missing, old values of <code>tagJson</code> will be used for the missing attributes
+     * instead.
      * 
      * @param tag
      *            may not be empty
-     * @param visible
-     *            default is <code>false</code>
      * 
      * @return status 204 (no content) if update was successful, otherwise 400 (bad request)
      * @see TagDTO
@@ -161,14 +162,26 @@ public class TagsResource extends AbstractSailingServerResource {
     public Response updateTag(@PathParam(RaceLogServletConstants.PARAMS_LEADERBOARD_NAME) String leaderboardName,
             @PathParam(RaceLogServletConstants.PARAMS_RACE_COLUMN_NAME) String raceColumnName,
             @PathParam(RaceLogServletConstants.PARAMS_RACE_FLEET_NAME) String fleetName,
-            @FormParam("tag_json") String tagJson, @FormParam("tag") String tag, @FormParam("comment") String comment,
-            @FormParam("image") String imageURL, @FormParam("public") boolean visibleForPublic) {
-        // TODO: What happens if parameters are missing? Old values should stay the same.
+            @FormParam("tag_json") String tagJson, @FormParam("tag") String tagParam,
+            @FormParam("comment") String commentParam, @FormParam("image") String imageURLParam,
+            @FormParam("public") String visibleForPublicParam) {
         Response response;
+        boolean successful = true;
         TagDTO tagToUpdate = serializer.deserializeTag(tagJson);
         TaggingService taggingService = getService().getTaggingService();
-        boolean successful = taggingService.updateTag(leaderboardName, raceColumnName, fleetName, tagToUpdate, tag,
-                comment, imageURL, visibleForPublic);
+
+        // only call update method when any of the parameters needs to be changed
+        if (tagParam != null || commentParam != null || imageURLParam != null || visibleForPublicParam != null) {
+            // keep old values when no new values are provided
+            String tag = tagParam == null ? tagToUpdate.getTag() : tagParam;
+            String comment = commentParam == null ? tagToUpdate.getComment() : commentParam;
+            String imageURL = imageURLParam == null ? tagToUpdate.getImageURL() : imageURLParam;
+            boolean visibleForPublic = visibleForPublicParam == null ? tagToUpdate.isVisibleForPublic()
+                    : visibleForPublicParam.equalsIgnoreCase("true") ? true : false;
+
+            successful = taggingService.updateTag(leaderboardName, raceColumnName, fleetName, tagToUpdate, tag, comment,
+                    imageURL, visibleForPublic);
+        }
         if (successful) {
             response = Response.noContent().build();
         } else {

java/com.sap.sailing.server.gateway/webservices/api/v1/tagsCRUDDoc.html

@@ -210,6 +210,7 @@
     <tr>
         <td>Optional parameters:</td>
         <td>
+            <p>When non of these parameters are provided the tag will not be changed. Missing parameters do not affect the tag. To reset a value provide the parameter with the value of an empty string.</p>
             tag: new title of tag<br/>
             comment: new comment<br/>
             image: new image URL<br/>

java/com.sap.sailing.server/src/com/sap/sailing/server/tagging/TaggingService.java

@@ -20,6 +20,7 @@ public interface TaggingService {
         RACELOG_NOT_FOUND("racelogNotFound", "Racelog not found"),
         TAG_NOT_REVOKABLE("tagNotRevokable", "This tag cannot be revoked"),
         TAG_ALREADY_EXISTS("tagAlreadyExists", "Tag does already exist, duplicated tags are not allowed"),
+        TAG_ALREADY_REMOVED("tagAlreadyRemoved", "Tag cannot be removed twice!"),
         TAG_NOT_EMPTY("tagNotEmpty", "Tag may not be empty"),
         TIMEPOINT_NOT_EMPTY("timepointNotEmpty", "Timepoint may not be empty"),
         TAG_TOO_LONG("tagTooLong", "Tag is too long"),

java/com.sap.sailing.server/src/com/sap/sailing/server/tagging/TaggingServiceImpl.java

@@ -148,26 +148,31 @@ public class TaggingServiceImpl implements TaggingService {
                         && tagEvent.getImageURL().equals(tag.getImageURL())
                         && tagEvent.getUsername().equals(tag.getUsername())
                         && tagEvent.getLogicalTimePoint().equals(tag.getRaceTimepoint())) {
-                    try {
-                        // TODO: As soon as permission-vertical branch got merged into master, apply
-                        // new permission system at this permission check (see bug 4104, comment 9)
-                        // functionality: Check if user has the permission to delete tag from RaceLog (same user or
-                        // admin).
-                        Subject subject = SecurityUtils.getSubject();
-                        subject.checkPermission(
-                                Permission.LEADERBOARD.getStringPermissionForObjects(Mode.UPDATE, leaderboardName));
-                        if ((subject.getPrincipal() != null && subject.getPrincipal().equals(tag.getUsername()))
-                                || subject.hasRole("admin")) {
-                            raceLog.revokeEvent(tagEvent.getAuthor(), tagEvent, "Revoked");
-                        } else {
+                    if (tag.getRevokedAt() == null || tag.getRevokedAt().asMillis() == 0) {
+                        try {
+                            // TODO: As soon as permission-vertical branch got merged into master, apply
+                            // new permission system at this permission check (see bug 4104, comment 9)
+                            // functionality: Check if user has the permission to delete tag from RaceLog (same user or
+                            // admin).
+                            Subject subject = SecurityUtils.getSubject();
+                            subject.checkPermission(
+                                    Permission.LEADERBOARD.getStringPermissionForObjects(Mode.UPDATE, leaderboardName));
+                            if ((subject.getPrincipal() != null && subject.getPrincipal().equals(tag.getUsername()))
+                                    || subject.hasRole("admin")) {
+                                raceLog.revokeEvent(tagEvent.getAuthor(), tagEvent, "Revoked");
+                            } else {
+                                setLastErrorCode(ErrorCode.MISSING_PERMISSIONS);
+                                successful = false;
+                            }
+                        } catch (AuthorizationException e) {
                             setLastErrorCode(ErrorCode.MISSING_PERMISSIONS);
                             successful = false;
+                        } catch (NotRevokableException e) {
+                            setLastErrorCode(ErrorCode.TAG_NOT_REVOKABLE);
+                            successful = false;
                         }
-                    } catch (AuthorizationException e) {
-                        setLastErrorCode(ErrorCode.MISSING_PERMISSIONS);
-                        successful = false;
-                    } catch (NotRevokableException e) {
-                        setLastErrorCode(ErrorCode.TAG_NOT_REVOKABLE);
+                    } else {
+                        setLastErrorCode(ErrorCode.TAG_ALREADY_REMOVED);
                         successful = false;
                     }
                     break;