Gh897 refactoring extract change status class #1003

KrivenkoAlexander · 2019-02-09T19:33:12Z

Addressed to #897

mystamps-bot · 2019-02-09T19:43:59Z

	1 Message
📖	@KrivenkoAlexander thank you for the PR! All quality checks have been passed! Next step is to wait when @php-coder will review this code

Generated by 🚫 Danger

php-coder · 2019-02-09T23:43:47Z

@KrivenkoAlexander #1004 has been fixed but it doesn't take an action here because your branch doesn't contain this fix. Could you fetch new changes from my master and rebase your branch on top of updated master? Let me know if you need help with that, but typically these commands work:

$ git checkout master
$ git fetch upstream
$ git merge upstream/master
$ git rebase master  gh897_refactoring_extract_changeStatus_class
$ git push --force-with-lease

php-coder

Thank you for the pull request!

I left my comments. Also, after rebase, a bot will complain on small issues. Please, take a look and fix them.

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

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

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

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

php-coder · 2019-02-10T00:44:30Z

Complains from FindBugs you can exclude for now. See example:

mystamps/src/main/config/findbugs-filter.xml

Lines 81 to 84 in c4eed77

    
           <Match> 
        
           	<Class name="ru.mystamps.web.feature.series.SitemapInfoDto" /> 
        
           	<Bug pattern="EI_EXPOSE_REP,EI_EXPOSE_REP2" /> 
        
           </Match>

php-coder · 2019-02-10T23:21:11Z

@KrivenkoAlexander Could you update master and rebase your branch again, please? I've edited this file also and this change produces a conflict. :-|

php-coder · 2019-02-11T10:57:44Z

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

+ */
+package ru.mystamps.web.feature.series.importing;
+
+


Please, remove one extra line here.

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

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

php-coder · 2019-02-12T19:49:46Z

See errors from codenarc, plus my comments, plus squash all the commits into one (see a link in the message from a bot).

php-coder · 2019-02-12T19:50:22Z

P.S. Please, ping me when it will be ready for review/merge.

codecov · 2019-02-13T10:52:12Z

Codecov Report

Merging #1003 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1003      +/-   ##
============================================
+ Coverage     75.95%   75.97%   +0.01%     
  Complexity      487      487              
============================================
  Files            37       37              
  Lines          1539     1540       +1     
  Branches        197      197              
============================================
+ Hits           1169     1170       +1     
  Misses          354      354              
  Partials         16       16

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

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 5cbd398...192187c. Read the comment docs.

codecov · 2019-02-13T10:52:13Z

Codecov Report

Merging #1003 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1003      +/-   ##
============================================
+ Coverage     75.95%   75.97%   +0.01%     
  Complexity      487      487              
============================================
  Files            37       37              
  Lines          1539     1540       +1     
  Branches        197      197              
============================================
+ Hits           1169     1170       +1     
  Misses          354      354              
  Partials         16       16

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

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 5cbd398...248b985. Read the comment docs.

php-coder · 2019-02-13T11:03:01Z

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

@@ -101,7 +101,7 @@ class SeriesImportServiceImplTest extends Specification {
 			ex.message == 'User id must be non null'
 	}

-	def 'addRequest() should throw exception if url is incorrect'() {
+		def 'addRequest() should throw exception if url is incorrect'() {


Could you revert the change in this line? Indentation should stay intact.

php-coder · 2019-02-13T11:03:18Z

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

@@ -367,7 +366,7 @@ class SeriesImportServiceImplTest extends Specification {
 			IllegalArgumentException ex = thrown()
 			ex.message == 'Request id must be non null'
 	}
-	
+


Please, don't change anything unrelated to your fix.

Still see it there btw.

php-coder · 2019-02-13T11:06:26Z

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

-				expectedRequestId,
-				{ Date date ->
-					assert DateUtils.roughlyEqual(date, new Date())
+			1 * seriesImportDao.changeStatus { UpdateImportRequestStatusDbDto status  ->


status ->

Could you remove double space here?

php-coder · 2019-02-13T11:11:28Z

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

-				expectedOldStatus,
-				expectedNewStatus
-			)
+			1 * seriesImportDao.changeStatus { UpdateImportRequestStatusDbDto status ->


In order to be consistent with other code, could you use brackets so the invocation will look close to the function invocation? It should be fixed in this case and others that you've modified. In other words, please, replace

1 * class.method { Dto dto -> ... }

by

1 * class.method({ Dto dto -> ... })

I did so just to resolve the codenarch violations. Take a look at

CodeNarcRunner [main] DEBUG - results=DirectoryResults(null) [DirectoryResults() [DirectoryResults(ru) [DirectoryResults(ru/mystamps) [DirectoryResults(ru/mystamps/web) [DirectoryResults(ru/mystamps/web/util) [], DirectoryResults(ru/mystamps/web/feature) [DirectoryResults(ru/mystamps/web/feature/series) [DirectoryResults(ru/mystamps/web/feature/series/sale) [], DirectoryResults(ru/mystamps/web/feature/series/importing) [DirectoryResults(ru/mystamps/web/feature/series/importing/sale) [], FileResults(ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy) [Violation[rule=ClosureAsLastMethodParameterRule[name=ClosureAsLastMethodParameter, priority=3], lineNumber=321, sourceLine=1 * seriesImportDao.changeStatus ({ UpdateImportRequestStatusDbDto status ->, message=Violation in class ru.mystamps.web.feature.series.importing.SeriesImportServiceImplTest. The last parameter to the 'changeStatus' method call is a closure an can appear outside the parenthesis], Violation[rule=ClosureAsLastMethodParameterRule[name=ClosureAsLastMethodParameter, priority=3], lineNumber=412, sourceLine=1 * seriesImportDao.changeStatus ({ UpdateImportRequestStatusDbDto status ->, message=Violation in class ru.mystamps.web.feature.series.importing.SeriesImportServiceImplTest. The last parameter to the 'changeStatus' method call is a closure an can appear outside the parenthesis], Violation[rule=ClosureAsLastMethodParameterRule[name=ClosureAsLastMethodParameter, priority=3], lineNumber=568, sourceLine=1 * seriesImportDao.changeStatus ({ UpdateImportRequestStatusDbDto status ->, message=Violation in class ru.mystamps.web.feature.series.importing.SeriesImportServiceImplTest. The last parameter to the 'changeStatus' method call is a closure an can appear outside the parenthesis]]]], DirectoryResults(ru/mystamps/web/feature/collection) [], DirectoryResults(ru/mystamps/web/feature/category) [], DirectoryResults(ru/mystamps/web/feature/participant) [], DirectoryResults(ru/mystamps/web/feature/image) [], DirectoryResults(ru/mystamps/web/feature/account) [], DirectoryResults(ru/mystamps/web/feature/country) []], DirectoryResults(ru/mystamps/web/service) []]]]]]

it is saying to me that we have 3 violation .
[INFO] totalPriority1Violations is 0 [INFO] totalPriority2Violations is 0 [INFO] totalPriority3Violations is 3
I am afraid that build won't pass because of it . Am i wrong?

Thank you, I got it now.

Yes, we have to turn off this warnings with an annotation. See example:

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

Line 115 in 5cbd398

@SuppressWarnings(['ClosureAsLastMethodParameter', 'UnnecessaryReturnKeyword'])

i see, i've done. Thanks

php-coder · 2019-02-13T11:12:03Z

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

-				SeriesImportRequestStatus.PARSING_SUCCEEDED
-			)
+			}
+


One more extra line should be removed.

php-coder · 2019-02-13T11:13:20Z

Some minor formatting issues and I think we I will merge it :) Thank you for your patience!

php-coder

Looks good to me, thank you! I'll merge it tonight (with or without 2 tiny whitespace-related changes).

KrivenkoAlexander · 2019-02-13T13:11:14Z

Sounds good to me :) It is worth to celebrate it , definitely . Thanks you. If you don't mind i will continue to work :) I need learn groovy a little bit and spock frame work . I haven't worked with them before. ср, 13 февр. 2019 г. в 15:58, Slava Semushin <[email protected]>:

…

***@***.**** approved this pull request. Looks good to me, thank you! I'll merge it tonight (with or without 2 tiny whitespaced-related changes). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1003 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKy3b_eJVQRmOC4AGG575SWQGDYMhbhjks5vNAv-gaJpZM4ays29> .

-- С уважением, Кривенко Александр! [email protected] skype wyrik88

php-coder · 2019-02-13T14:12:57Z

If you don't mind i will continue to work :)

Yes, sure. Let me know when you will be ready for the next task then.

I need learn groovy a little bit and spock frame work .

Only if it's interested to you because in this project we don't have code that is more complicated than you have seen already.

php-coder · 2019-02-13T14:16:01Z

I need learn groovy a little bit and spock frame work

P.S. These links could be useful to you:

KrivenkoAlexander · 2019-02-13T17:21:50Z

Hi.Thank you for link that you sent me. I will look at them. You can send me tasks as soon as possible. I will familiarise with it. Understanding and clarification the tasks can take time. That why it better ti start as soon as possible. About groovy: yes, i want to clarify some basics for myself to save time in future work. Thanks. -- Отправлено из myMail для Android среда, 13 февраля 2019г., 17:12 +03:00 от Slava Semushin [email protected] :

…

>If you don't mind i will continue to work :) Yes, sure. Let me know when you will be ready for the next task then. >I need learn groovy a little bit and spock frame work . Only if it's interested to you because in this project we don't have code that is more complicated than you have seen already. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .

php-coder · 2019-02-13T19:14:51Z

Merged in e32b74c commit.

php-coder · 2019-02-13T19:20:37Z

@KrivenkoAlexander Thank you one more time for the contribution! 🥇

I've added you to the board: https://github.com/php-coder/mystamps/wiki/team-members :)

Also here is the steps to follow after merging PRs: https://github.com/php-coder/mystamps/wiki/after_merge_steps

See you in the next tasks! ;-)

KrivenkoAlexander requested a review from php-coder as a code owner February 9, 2019 19:33

php-coder mentioned this pull request Feb 9, 2019

Invalid Dangerfile file: undefined method text for nil:NilClass #1004

Closed

php-coder requested changes Feb 10, 2019

View reviewed changes

KrivenkoAlexander force-pushed the gh897_refactoring_extract_changeStatus_class branch from a7c1130 to dfee630 Compare February 10, 2019 15:35

php-coder reviewed Feb 11, 2019

View reviewed changes

KrivenkoAlexander force-pushed the gh897_refactoring_extract_changeStatus_class branch from 47c0b8d to 7b28a38 Compare February 12, 2019 18:40

php-coder reviewed Feb 12, 2019

View reviewed changes

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

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

php-coder reviewed Feb 12, 2019

View reviewed changes

src/test/groovy/ru/mystamps/web/feature/series/importing/SeriesImportServiceImplTest.groovy Outdated Show resolved Hide resolved

KrivenkoAlexander force-pushed the gh897_refactoring_extract_changeStatus_class branch from 7b28a38 to 192187c Compare February 13, 2019 10:40

php-coder reviewed Feb 13, 2019

View reviewed changes

Extract the method from 4 vars in order to use DbDto object

248b985

KrivenkoAlexander force-pushed the gh897_refactoring_extract_changeStatus_class branch from 192187c to 248b985 Compare February 13, 2019 12:35

php-coder approved these changes Feb 13, 2019

View reviewed changes

php-coder closed this Feb 13, 2019

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

KrivenkoAlexander deleted the gh897_refactoring_extract_changeStatus_class branch February 14, 2019 14:21

		*/
		package ru.mystamps.web.feature.series.importing;

Gh897 refactoring extract change status class #1003

Gh897 refactoring extract change status class #1003

Uh oh!

Conversation

KrivenkoAlexander commented Feb 9, 2019 • edited by php-coder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mystamps-bot commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

php-coder commented Feb 10, 2019

Uh oh!

php-coder commented Feb 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

php-coder commented Feb 12, 2019

Uh oh!

php-coder commented Feb 12, 2019

Uh oh!

codecov bot commented Feb 13, 2019

Codecov Report

Uh oh!

codecov bot commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Feb 13, 2019

Uh oh!

php-coder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

KrivenkoAlexander commented Feb 9, 2019 •

edited by php-coder

Loading

mystamps-bot commented Feb 9, 2019 •

edited

Loading

php-coder commented Feb 9, 2019 •

edited

Loading

codecov bot commented Feb 13, 2019 •

edited

Loading

php-coder left a comment •

edited

Loading

php-coder commented Feb 13, 2019 •

edited

Loading