Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object #1006

KrivenkoAlexander · 2019-02-16T13:53:13Z

Addressed to #756

mystamps-bot · 2019-02-16T14:03:23Z

	1 Warning
⚠️	danger check: branch `Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object` does not comply with our best practices. Branch name should use the following scheme: `ghXXX_meaningful-name` where `XXX` is an issue number. Next time, please, use this scheme :)

Generated by 🚫 Danger

codecov · 2019-02-16T14:04:09Z

Codecov Report

Merging #1006 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master   #1006      +/-   ##
===========================================
+ Coverage     79.93%     80%   +0.06%     
- Complexity      488     489       +1     
===========================================
  Files            35      35              
  Lines          1465    1465              
  Branches        188     188              
===========================================
+ Hits           1171    1172       +1     
  Misses          277     277              
+ Partials         17      16       -1

Impacted Files	Coverage Δ	Complexity Δ
...ture/series/importing/SeriesImportServiceImpl.java	`97.89% <ø> (+1.05%)`	`31 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b23d67...fa8ecc3. Read the comment docs.

codecov · 2019-02-16T14:04:09Z

Codecov Report

Merging #1006 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1006      +/-   ##
============================================
+ Coverage     79.93%   80.01%   +0.08%     
- Complexity      488      489       +1     
============================================
  Files            35       35              
  Lines          1465     1466       +1     
  Branches        188      188              
============================================
+ Hits           1171     1173       +2     
  Misses          277      277              
+ Partials         17       16       -1

Impacted Files	Coverage Δ	Complexity Δ
...ture/series/importing/SeriesImportServiceImpl.java	`97.91% <100%> (+1.07%)`	`31 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b23d67...4a08dd1. Read the comment docs.

php-coder

Thank you!

LGTM overall but I left a couple comments anyway.

php-coder · 2019-02-16T22:00:55Z

src/main/java/ru/mystamps/web/feature/series/importing/JdbcSeriesImportDao.java

@@ -97,26 +96,21 @@ public Integer add(ImportSeriesDbDto importRequest) {

 	// @todo #735 SeriesImportDao.setSeriesIdAndChangeStatus(): replace arguments by dto object


We need to remove this comment altogether.

php-coder · 2019-02-16T22:02:22Z

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy

 					return true
-				}
+				}				


Please, remove trailing spaces/tabs.

php-coder · 2019-02-16T22:03:50Z

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy

-				SeriesImportRequestStatus.IMPORT_SUCCEEDED,
-				{ Date updatedAt ->
-					assert DateUtils.roughlyEqual(updatedAt, new Date())
+				{ UpdateImportRequestStatusDbDto request ->


Let's rename it to status (for consistency with the tests for changeStatus()).

php-coder · 2019-02-16T22:05:17Z

src/main/java/ru/mystamps/web/feature/series/importing/SeriesImportServiceImpl.java

@@ -98,7 +97,6 @@ public Integer addSeries(
 		AddSeriesSalesDto saleDto,
 		Integer requestId,
 		Integer userId) {
-


Please, don't change anything that isn't related to the issue. The commit should be focused only on a single change.

it was reverted back . Sorry, it is my mad :(

php-coder · 2019-02-16T22:06:14Z

src/main/java/ru/mystamps/web/feature/series/importing/JdbcSeriesImportDao.java

@@ -25,7 +25,6 @@
 import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
 import org.springframework.jdbc.support.GeneratedKeyHolder;
 import org.springframework.jdbc.support.KeyHolder;
-


I'm surprised that checkstyle doesn't complain about this change... How about reverting it?

I am surprised too .Because i remember that i had to change such order in previous PRs

php-coder · 2019-02-16T22:07:58Z

src/main/java/ru/mystamps/web/feature/series/importing/SeriesImportServiceImpl.java

-			SeriesImportRequestStatus.PARSING_SUCCEEDED,
-			SeriesImportRequestStatus.IMPORT_SUCCEEDED,
-			now
+			new UpdateImportRequestStatusDbDto(


It's better to follow the same approach everywhere. See how we have done the same in the changeStatus():

UpdateImportRequestStatusDbDto status = new UpdateImportRequestStatusDbDto(requestId, now, oldStatus, newStatus);

Let's do the same here.

Done . I explicit created the status variable and added it to the method call.

php-coder · 2019-02-16T22:10:31Z

src/main/java/ru/mystamps/web/feature/series/importing/JdbcSeriesImportDao.java

-		Date updatedAt) {
-
+	public void setSeriesIdAndChangeStatus(Integer seriesId,
+										   UpdateImportRequestStatusDbDto requestStatus) {


We don't format methods like that. See how the arguments wrapped in other places and follow the style.

Another option (if the line just a little long or you need to have everything in one like to improve readability) -- add a special comment and suppress checkstyle complains.

Done. I used suppress annotation for "linelength" module

php-coder · 2019-02-16T22:10:59Z

src/main/java/ru/mystamps/web/feature/series/importing/SeriesImportServiceImpl.java

@@ -37,7 +37,6 @@
 import ru.mystamps.web.feature.series.sale.SeriesSalesService;
 import ru.mystamps.web.support.spring.security.HasAuthority;
 import ru.mystamps.web.util.CatalogUtils;
-


Please, revert this change.

… made and the refactoring for codestyle was made here either

php-coder · 2019-02-17T20:54:40Z

Merged in 86042dc commit.

php-coder · 2019-02-17T20:56:47Z

@KrivenkoAlexander Thank you for the contribution! 👍

KrivenkoAlexander requested a review from php-coder as a code owner February 16, 2019 13:53

php-coder requested changes Feb 16, 2019

View reviewed changes

Changes for review php-coder#1: replacing arguments by dto object was…

4a08dd1

… made and the refactoring for codestyle was made here either

KrivenkoAlexander force-pushed the Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object branch from fa8ecc3 to 4a08dd1 Compare February 17, 2019 18:44

php-coder closed this Feb 17, 2019

php-coder added the resolution/merged label Feb 17, 2019

KrivenkoAlexander deleted the Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object branch February 18, 2019 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object #1006

Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object #1006

KrivenkoAlexander commented Feb 16, 2019

mystamps-bot commented Feb 16, 2019

codecov bot commented Feb 16, 2019

codecov bot commented Feb 16, 2019 •

edited

Loading

php-coder left a comment

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder Feb 16, 2019

KrivenkoAlexander Feb 17, 2019

php-coder commented Feb 17, 2019

php-coder commented Feb 17, 2019

		@@ -97,26 +96,21 @@ public Integer add(ImportSeriesDbDto importRequest) {

		// @todo #735 SeriesImportDao.setSeriesIdAndChangeStatus(): replace arguments by dto object

Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object #1006

Gh756_SeriesImportDao_setSeriesIdAndChangeStatus_replace_arguments_by_dto_object #1006

Conversation

KrivenkoAlexander commented Feb 16, 2019

mystamps-bot commented Feb 16, 2019

codecov bot commented Feb 16, 2019

Codecov Report

codecov bot commented Feb 16, 2019 • edited Loading

Codecov Report

php-coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

php-coder commented Feb 17, 2019

php-coder commented Feb 17, 2019

codecov bot commented Feb 16, 2019 •

edited

Loading