feat: admin can add a comment to a series.

php-coder · php-coder · commit 108acf7988df · 2020-05-24T22:54:56.000+01:00
Implementation details: a comment can be added only if there is no comment yet. This is a protection against possible race or other unforeseeable circumstances that might lead to data loss. A full implementation of the editing can be added later so this can be considered as the first iteration. Fix #785
diff --git a/NEWS.txt b/NEWS.txt
@@ -2,6 +2,7 @@
 - (feature) a series can be marked as a similar to another one
 - (feature) series images can be replaced
 - (feature) add preliminary support for hidden images
+- (feature) admin can add a comment to a series
 
 0.4.3
 - (feature)        add support for Ukrainian hryvnia
diff --git a/src/main/frontend/src/components/AddCommentForm.js b/src/main/frontend/src/components/AddCommentForm.js
@@ -54,8 +54,8 @@ class AddCommentForm extends React.PureComponent {
 
 				if (data.hasOwnProperty('fieldErrors')) {
 					const fieldErrors = [];
-					if (data.fieldErrors.comment) {
-						fieldErrors.push(...data.fieldErrors.comment);
+					if (data.fieldErrors.value) {
+						fieldErrors.push(...data.fieldErrors.value);
 					}
 					this.setState({
 						isDisabled: false,
diff --git a/src/main/java/ru/mystamps/web/feature/series/JdbcSeriesDao.java b/src/main/java/ru/mystamps/web/feature/series/JdbcSeriesDao.java
@@ -48,6 +48,9 @@ public class JdbcSeriesDao implements SeriesDao {
 	@Value("${series.create}")
 	private String createSeriesSql;
 	
+	@Value("${series.add_comment}")
+	private String addCommentSql;
+	
 	@Value("${series.mark_as_modified}")
 	private String markAsModifiedSql;
 	
@@ -136,6 +139,22 @@ public Integer add(AddSeriesDbDto series) {
 		return Integer.valueOf(holder.getKey().intValue());
 	}
 	
+	@Override
+	public void addComment(Integer seriesId, String comment) {
+		Map<String, Object> params = new HashMap<>();
+		params.put("series_id", seriesId);
+		params.put("comment", comment);
+		
+		int affected = jdbcTemplate.update(addCommentSql, params);
+		
+		// @todo #785 Update series: handle refuse to update an existing comment gracefully
+		Validate.validState(
+			affected == 1,
+			"Unexpected number of affected rows after updating series: %d",
+			affected
+		);
+	}
+	
 	/**
 	 * @author Sergey Chechenev
 	 */
diff --git a/src/main/java/ru/mystamps/web/feature/series/RestSeriesController.java b/src/main/java/ru/mystamps/web/feature/series/RestSeriesController.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2009-2020 Slava Semushin <slava.semushin@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+package ru.mystamps.web.feature.series;
+
+import lombok.RequiredArgsConstructor;
+import org.springframework.http.ResponseEntity;
+import org.springframework.validation.annotation.Validated;
+import org.springframework.web.bind.annotation.PatchMapping;
+import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.annotation.RequestBody;
+import org.springframework.web.bind.annotation.RestController;
+import ru.mystamps.web.support.spring.mvc.PatchRequest;
+import ru.mystamps.web.support.spring.mvc.PatchRequest.Operation;
+
+import javax.servlet.http.HttpServletResponse;
+import javax.validation.Valid;
+import javax.validation.constraints.NotEmpty;
+import java.io.IOException;
+import java.util.List;
+
+@Validated
+@RestController
+@RequiredArgsConstructor
+class RestSeriesController {
+	
+	private final SeriesService seriesService;
+	
+	// @todo #785 Update series: add integration test
+	// @todo #785 Update series: add validation for a comment
+	@PatchMapping(SeriesUrl.INFO_SERIES_PAGE)
+	@SuppressWarnings("PMD.TooFewBranchesForASwitchStatement")
+	public ResponseEntity<Void> updateSeries(
+		@PathVariable("id") Integer seriesId,
+		@RequestBody @Valid @NotEmpty List<@Valid PatchRequest> patches,
+		HttpServletResponse response) throws IOException {
+		
+		if (seriesId == null) {
+			response.sendError(HttpServletResponse.SC_NOT_FOUND);
+			return null;
+		}
+		
+		if (!seriesService.isSeriesExist(seriesId)) {
+			response.sendError(HttpServletResponse.SC_NOT_FOUND);
+			return null;
+		}
+		
+		for (PatchRequest patch : patches) {
+			if (patch.op != Operation.add) {
+				// @todo #785 Update series: properly fail on non-supported operations
+				continue;
+			}
+			
+			switch (patch.path) {
+				case "/comment":
+					seriesService.addComment(seriesId, patch.value);
+					break;
+				default:
+					// @todo #785 Update series: properly fail on invalid path
+					break;
+			}
+		}
+		
+		return ResponseEntity.noContent().build();
+	}
+	
+}
+
diff --git a/src/main/java/ru/mystamps/web/feature/series/SeriesConfig.java b/src/main/java/ru/mystamps/web/feature/series/SeriesConfig.java
@@ -69,6 +69,11 @@ public SeriesController seriesController() {
 			);
 		}
 		
+		@Bean
+		public RestSeriesController restSeriesController() {
+			return new RestSeriesController(seriesService);
+		}
+		
 	}
 	
 	@Import(Daos.class)
diff --git a/src/main/java/ru/mystamps/web/feature/series/SeriesDao.java b/src/main/java/ru/mystamps/web/feature/series/SeriesDao.java
@@ -24,6 +24,7 @@
 @SuppressWarnings("PMD.TooManyMethods")
 public interface SeriesDao {
 	Integer add(AddSeriesDbDto series);
+	void addComment(Integer seriesId, String comment);
 	void markAsModified(Integer seriesId, Date updateAt, Integer updatedBy);
 	List<SitemapInfoDto> findAllForSitemap();
 	List<SeriesLinkDto> findSimilarSeries(Integer seriesId, String lang);
diff --git a/src/main/java/ru/mystamps/web/feature/series/SeriesService.java b/src/main/java/ru/mystamps/web/feature/series/SeriesService.java
@@ -24,6 +24,7 @@
 @SuppressWarnings("PMD.TooManyMethods")
 public interface SeriesService {
 	Integer add(AddSeriesDto dto, Integer userId, boolean userCanAddComments);
+	void addComment(Integer seriesId, String comment);
 	void addImageToSeries(AddImageDto dto, Integer seriesId, Integer userId);
 	void replaceImage(ReplaceImageDto dto, Integer seriesId, Integer userId);
 	long countAll();
diff --git a/src/main/java/ru/mystamps/web/feature/series/SeriesServiceImpl.java b/src/main/java/ru/mystamps/web/feature/series/SeriesServiceImpl.java
@@ -118,6 +118,23 @@ public Integer add(AddSeriesDto dto, Integer userId, boolean userCanAddComments)
 		return id;
 	}
 
+	// @todo #785 SeriesServiceImpl.addComment(): add unit tests
+	@Override
+	@Transactional
+	@PreAuthorize(HasAuthority.ADD_COMMENTS_TO_SERIES)
+	public void addComment(Integer seriesId, String comment) {
+		Validate.isTrue(seriesId != null, "Series id must be non null");
+		
+		// We don't touch updated_at/updated_by fields because:
+		// - a comment is a meta information that is visible only by admins.
+		//   From user's point of view, this field doesn't exist
+		// - updated_at field is used by logic for sitemap.xml generation
+		//   and we don't want to affect this
+		seriesDao.addComment(seriesId, comment);
+		
+		log.info("Series #{}: a comment has been added", seriesId);
+	}
+	
 	@Override
 	@Transactional
 	@PreAuthorize("isAuthenticated()")
diff --git a/src/main/java/ru/mystamps/web/feature/site/ResourceUrl.java b/src/main/java/ru/mystamps/web/feature/site/ResourceUrl.java
@@ -32,7 +32,7 @@ public final class ResourceUrl {
 	public static final String STATIC_RESOURCES_URL = "https://stamps.filezz.ru";
 	
 	// MUST be updated when any of our resources were modified
-	public static final String RESOURCES_VERSION = "v0.4.3.8";
+	public static final String RESOURCES_VERSION = "v0.4.3.9";
 
 	// CheckStyle: ignore LineLength for next 15 lines
 	private static final String CATALOG_UTILS_JS      = "/public/js/" + RESOURCES_VERSION + "/CatalogUtils.min.js";
diff --git a/src/main/java/ru/mystamps/web/support/spring/mvc/PatchRequest.java b/src/main/java/ru/mystamps/web/support/spring/mvc/PatchRequest.java
@@ -0,0 +1,50 @@
+/*
+ * Copyright (C) 2009-2020 Slava Semushin <slava.semushin@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+package ru.mystamps.web.support.spring.mvc;
+
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+
+import javax.validation.constraints.NotEmpty;
+import javax.validation.constraints.NotNull;
+
+// See for details: http://jsonpatch.com
+@Getter
+@Setter
+@ToString
+public class PatchRequest {
+
+	public enum Operation {
+		add, copy, move, remove, replace, test;
+	}
+
+	// @todo #785 Update series: add integration test for required "op" field
+	@NotNull
+	private Operation op;
+
+	// @todo #785 Update series: add integration test for non-empty "path" field
+	@NotEmpty
+	private String path;
+
+	// @todo #785 Update series: add integration test for non-empty "value" field
+	@NotEmpty
+	private String value;
+
+}
diff --git a/src/main/java/ru/mystamps/web/support/spring/mvc/RestExceptionHandler.java b/src/main/java/ru/mystamps/web/support/spring/mvc/RestExceptionHandler.java
@@ -25,6 +25,8 @@
 import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.bind.annotation.RestControllerAdvice;
 
+import javax.validation.ConstraintViolationException;
+
 @RestControllerAdvice
 public class RestExceptionHandler {
 	
@@ -45,4 +47,20 @@ public ResponseEntity<ValidationErrors> handleMethodArgumentNotValidException(
 		);
 	}
 	
+	// handle cases like "@RequestBody @Valid @NotEmpty List<@Valid PatchRequest> patches"
+	@ExceptionHandler(ConstraintViolationException.class)
+	public ResponseEntity<ValidationErrors> handleConstraintViolationException(
+		ConstraintViolationException ex) {
+		
+		if (ex == null) {
+			LOG.warn("Couldn't handle ConstraintViolationException that is null");
+			return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
+		}
+		
+		return new ResponseEntity<>(
+			new ValidationErrors(ex.getConstraintViolations()),
+			HttpStatus.BAD_REQUEST
+		);
+	}
+	
 }
diff --git a/src/main/java/ru/mystamps/web/support/spring/mvc/ValidationErrors.java b/src/main/java/ru/mystamps/web/support/spring/mvc/ValidationErrors.java
@@ -21,10 +21,14 @@
 import org.springframework.validation.Errors;
 import org.springframework.validation.FieldError;
 
+import javax.validation.ConstraintViolation;
+import javax.validation.ElementKind;
+import javax.validation.Path.Node;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 @Getter
 public class ValidationErrors {
@@ -36,15 +40,48 @@ public ValidationErrors(Errors errors) {
 			.forEach(this::extractFieldError);
 	}
 	
+	public ValidationErrors(Set<ConstraintViolation<?>> violations) {
+		violations.forEach(this::extractFieldError);
+	}
+	
 	private void extractFieldError(FieldError error) {
 		String name    = error.getField();
 		String message = error.getDefaultMessage();
 		
 		getErrorsByFieldName(name).add(message);
 	}
 	
+	// @todo #785 Improve error handling for requests with a list of objects
+	private void extractFieldError(ConstraintViolation<?> violation) {
+		// Extract a field name from a path: updateSeries.patches[0].value -> value
+		// In this case, we loose index (see Node.getIndex() and Node.isInIterable()) but
+		// as now we always have requests with a single object, that's not severe.
+		// Ideally, for multiple objects we should return ValidationErrors[] instead of
+		// ValidationErrors
+		Node last = getLastOrNull(violation.getPropertyPath());
+		String name;
+		if (last == null || last.getKind() != ElementKind.PROPERTY) {
+			// fallback to a field path for unsupported kinds
+			name = violation.getPropertyPath().toString();
+		} else {
+			name = last.getName();
+		}
+		
+		String message = violation.getMessage();
+		
+		getErrorsByFieldName(name).add(message);
+	}
+	
 	private List<String> getErrorsByFieldName(String name) {
 		return fieldErrors.computeIfAbsent(name, key -> new ArrayList<>());
 	}
 	
+	private static <T> T getLastOrNull(Iterable<T> elements) {
+		T last = null;
+		for (T elem : elements) {
+			last = elem;
+		}
+		return last;
+	}
+	
 }
diff --git a/src/main/java/ru/mystamps/web/support/spring/security/HasAuthority.java b/src/main/java/ru/mystamps/web/support/spring/security/HasAuthority.java
@@ -20,6 +20,7 @@
 @SuppressWarnings({ "checkstyle:linelength", "PMD.AvoidDuplicateLiterals" })
 public final class HasAuthority {
 	// Constants sorted in an ascending order.
+	public static final String ADD_COMMENTS_TO_SERIES = "hasAuthority('" + StringAuthority.ADD_COMMENTS_TO_SERIES + "')";
 	public static final String ADD_PARTICIPANT = "hasAuthority('" + StringAuthority.ADD_PARTICIPANT + "')";
 	@SuppressWarnings("PMD.LongVariable")
 	public static final String ADD_SERIES_PRICE_AND_COLLECTION_OWNER_OR_VIEW_ANY_ESTIMATION =
diff --git a/src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java b/src/main/java/ru/mystamps/web/support/spring/security/SecurityConfig.java
@@ -109,6 +109,7 @@ protected void configure(HttpSecurity http) throws Exception {
 				.mvcMatchers(CountryUrl.ADD_COUNTRY_PAGE).hasAuthority(StringAuthority.CREATE_COUNTRY)
 				.mvcMatchers(ParticipantUrl.ADD_PARTICIPANT_PAGE).hasAuthority(StringAuthority.ADD_PARTICIPANT)
 				.mvcMatchers(SeriesUrl.ADD_SERIES_PAGE).hasAuthority(StringAuthority.CREATE_SERIES)
+				.mvcMatchers(HttpMethod.PATCH, SeriesUrl.INFO_SERIES_PAGE).hasAuthority(StringAuthority.ADD_COMMENTS_TO_SERIES)
 				.mvcMatchers(SeriesImportUrl.REQUEST_IMPORT_SERIES_PAGE).hasAuthority(StringAuthority.IMPORT_SERIES)
 				.mvcMatchers(SiteUrl.SITE_EVENTS_PAGE).hasAuthority(StringAuthority.VIEW_SITE_EVENTS)
 				.mvcMatchers(CategoryUrl.SUGGEST_SERIES_CATEGORY).hasAuthority(StringAuthority.CREATE_SERIES)
diff --git a/src/main/resources/sql/series_dao_queries.properties b/src/main/resources/sql/series_dao_queries.properties
@@ -41,6 +41,12 @@ VALUES \
      , :updated_by \
      )
 
+series.add_comment = \
+UPDATE series \
+   SET comment = :comment \
+ WHERE id = :series_id \
+   AND comment IS NULL
+
 series.mark_as_modified = \
 UPDATE series \
    SET updated_at = :updated_at \
diff --git a/src/main/webapp/WEB-INF/views/series/info.html b/src/main/webapp/WEB-INF/views/series/info.html
@@ -398,7 +398,7 @@ <h5 th:text="#{t_hidden_images}">Hidden images</h5>
 
 							<div id="add-catalog-numbers" sec:authorize="hasAuthority('CREATE_SERIES')"></div>
 							
-							<div id="add-comment" sec:authorize="hasAuthority('ADD_COMMENTS_TO_SERIES')"></div>
+							<div id="add-comment" sec:authorize="hasAuthority('ADD_COMMENTS_TO_SERIES')" th:if="${series.comment == null}"></div>
 							
 							<div id="similar-series" class="row" th:if="${not #lists.isEmpty(similarSeries)}">
 								<div class="col-sm-10 col-sm-offset-2">
@@ -1058,15 +1058,19 @@ <h5 th:text="#{t_add_info_who_selling_series}">Add info about selling/buying thi
 		/*/-->
 		
 		<!--/*/
-		<th:block sec:authorize="hasAuthority('ADD_COMMENTS_TO_SERIES')">
+		<th:block sec:authorize="hasAuthority('ADD_COMMENTS_TO_SERIES')" th:if="${series.comment == null}">
 		/*/-->
 		<script src="../../../../../../target/classes/js/components/AddCommentForm.js" th:src="${ADD_COMMENT_FORM_JS}"></script>
 		<script th:inline="javascript">
 			/*[+
 			var addCommentProps = {
+				'url': [[ '__@{${INFO_SERIES_PAGE}(id=${series.id})}__' ]],
 				'csrfHeaderName': [[ ${_csrf.headerName} ]],
 				'csrfTokenValue': [[ ${_csrf.token} ]],
 				'l10n': {
+					't_server_error': [[ #{t_server_error} ]],
+					't_comment':      [[ #{t_comment} ]],
+					't_add':          [[ #{t_add} ]]
 				}
 			};
 			+]*/

Original file line number	Diff line number	Diff line change
`@@ -69,6 +69,11 @@ public SeriesController seriesController() {`
`69`	`69`	`);`
`70`	`70`	`}`
`71`	`71`
	`72`	`+ @Bean`
	`73`	`+ public RestSeriesController restSeriesController() {`
	`74`	`+ return new RestSeriesController(seriesService);`
	`75`	`+ }`
	`76`	`+`
`72`	`77`	`}`
`73`	`78`
`74`	`79`	`@Import(Daos.class)`