FINERACT-702: Integrate SpotBugs and fix over 300 high priority issues by awasum · Pull Request #670 · apache/fineract

awasum · 2019-12-10T16:51:05Z

FINERACT-702: Integrate SpotBugs and fix over 300 high priority rule Violations

This PR also serves as a base solution for FINERACT-712 and FINERACT-696

Review thoroughly

Description

https://issues.apache.org/jira/browse/FINERACT-702

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
API documentation at https://github.com/apache/fineract/blob/develop/api-docs/apiLive.htm has been updated with details of any API changes.
Integration tests have been created/updated for verifying the changes made.
All Integrations tests are passing with the new commits.
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)

Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide

…Violations This PR also serves as a base solution for FINERACT-712 and FINERACT-696 Review thoroughly

vorburger · 2019-12-10T20:11:10Z

WOW! Nice.. really nice. Please ping me back here when the build is passing (I haven't even looked at why it's failing), I'd love to review and merge this PR!

...ract-provider/src/main/java/org/apache/fineract/organisation/monetary/data/CurrencyData.java

…spotbugs)

…tbugs

vorburger · 2019-12-14T13:23:33Z

@awasum I was curious why this still failed, so I pulled it locally, and the docker compose did still work for me locally... not entirely sure what's going on here, but looks like perhaps it was just way too slow - Travis may have been really busy? I've raised #673, same thing (just squashed) plus slightly increased timeout - let's see if that one passes...

awasum · 2019-12-14T13:27:34Z

@awasum I was curious why this still failed, so I pulled it locally, and the docker compose did still work for me locally... not entirely sure what's going on here, but looks like perhaps it was just way too slow - Travis may have been really busy? I've raised #673, same thing (just squashed) plus slightly increased timeout - let's see if that one passes...

Looks..like the ShareAccount Integration tests are failing(locally) due to some equals or change I made while doing the spotbugs integration...I have not been able to pinpoint the problem further...But I will have a go at it again this evening.

Anyone reading this who could help pinpoint the problem before me..that will be great.

Last resort, I will revert the changes in the share account module and add one change at a time while run the integration tests till we get it.

vorburger

@awasum this is amazing work! I've done a detailed review, and have some feedback for you...

Just add additional commits, do you not yet squash, that makes it easier to re-review.

PS: Re. the build problem, with #673 having also failed, I'm now confused whether this PR is causing it, or it's a more general problem; let's see if a trivial #674 passes.

...src/integrationTest/java/org/apache/fineract/integrationtests/ClientLoanIntegrationTest.java

...vider/src/integrationTest/java/org/apache/fineract/integrationtests/common/CenterDomain.java

...rationTest/java/org/apache/fineract/integrationtests/common/accounting/GLAccountBuilder.java

fineract-provider/src/main/java/org/apache/fineract/accounting/common/AccountingConstants.java

...act-provider/src/main/java/org/apache/fineract/accounting/glaccount/domain/TrialBalance.java

...t-provider/src/main/java/org/apache/fineract/portfolio/paymenttype/data/PaymentTypeData.java

...rovider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java

...rc/main/java/org/apache/fineract/portfolio/self/loanaccount/data/SelfLoansDataValidator.java

fineract-provider/src/main/java/org/apache/fineract/template/service/TemplateMergeService.java

fineract-provider/src/test/java/org/apache/fineract/mix/report/service/XBRLBuilderTest.java

vorburger · 2019-12-14T14:54:59Z

Looks..like the ShareAccount Integration tests are failing(locally)

Oh! Silly me, I had not even noticed that (which is why I, confusingly, wrote about Docker above).

How about #675 to make this more obvious in the future? Both for stupid myself 😈 and others..

Anyone reading this who could help pinpoint the problem before me..that will be great.

See if any of the review comments I just posted (before reading this) may actually help to fix this?

Last resort, I will revert the changes in the share account module and add one change at a time while run the integration tests till we get it.

Yeah, I was thinking of suggesting doing some of the changes proposed here individually, but then didn't want to bother you with extra work. Especially those I picked up on, perhaps easier to get more eyes on them if smaller changes? Up to you.

…Violations This PR also serves as a base solution for FINERACT-712 and FINERACT-696 Review thoroughly

…spotbugs)

…otbugs

Merge commit '4524fbf' into spotbugs

awasum · 2019-12-22T15:11:40Z

@vorburger , If you have some time before xmas, will you like to review this again and merge if Ok? Travis build is now working.

awasum · 2019-12-22T15:12:30Z

@vorburger , If you have some time before xmas, will you like to review this again and merge if Ok? Travis build is now working.

Maybe, its time I squashed this?

vorburger · 2020-01-05T17:52:03Z

@awasum FYI I haven't forgotten about this, I'm hoping to make time to re-review some time in the coming days!

vorburger · 2020-01-10T16:00:32Z

.travis.yml

 # and Docker test does not run if integration test fails, which makes PR failure easier to understand.
 # @see https://docs.travis-ci.com/user/job-lifecycle/#customizing-the-build-phase
-  - ./gradlew --console=plain licenseMain licenseTest licenseIntegrationTest test  &&  ./gradlew --console=plain integrationTest  &&  sudo service mysql stop  &&  docker-compose build  &&  docker-compose up -d  &&  sleep 30s  &&  http --verify=no --timeout 240 --check-status get https://localhost:8443/fineract-provider/actuator/health || docker logs fineract_fineract-server_1
+  - ./gradlew --console=plain licenseMain licenseTest licenseIntegrationTest check  &&  ./gradlew --console=plain integrationTest  &&  sudo service mysql stop  &&  docker-compose build  &&  docker-compose up -d  &&  sleep 30s  &&  http --verify=no --timeout 240 --check-status get https://localhost:8443/fineract-provider/actuator/health || docker logs fineract_fineract-server_1


Changing gradle test to check is due to https://stackoverflow.com/questions/50104666/gradle-difference-between-test-and-check and looks great here! (Just recording this so that we remember it in the future.)

vorburger · 2020-01-10T16:14:14Z

@awasum I'd like to get this in (sorry my delay-...), so I'm going to go about reviewing it differently - I'll pull your branch, squash all of your work into a single commit (with you as the commit's author!), quickly resolve the (trivial) merge conflict caused by #678 myself, and then add one single new commit with changes I would have suggested you make in the final round of review, and then raise a new PR for you to re-review and self-merge - hope that's OK with you?

awasum · 2020-01-10T17:09:32Z

@awasum I'd like to get this in (sorry my delay-...), so I'm going to go about reviewing it differently - I'll pull your branch, squash all of your work into a single commit (with you as the commit's author!), quickly resolve the (trivial) merge conflict caused by #678 myself, and then add one single new commit with changes I would have suggested you make in the final round of review, and then raise a new PR for you to re-review and self-merge - hope that's OK with you?

Thanks. Go ahead and do it. I have been waiting for this so i can do the other related checkstyle, pmd and other quality checks.

vorburger · 2020-01-10T23:24:20Z

...egrationTest/java/org/apache/fineract/integrationtests/LoanReschedulingWithinCenterTest.java

        // VERIFY THE INTEREST
        Float interestDue = (Float) ((HashMap) loanRepaymnetSchedule.get(2)).get("interestDue");
-        assertEquals(String.valueOf(interestDue), "90.82");
+        assertEquals(String.valueOf(interestDue), "42.38");


As per #682, this, and the other change in line 283 below, doesn't seem right...

vorburger · 2020-01-10T23:38:27Z

@awasum if you don't mind I would suggest that we (I'll let you) close this PR, and continue (together) based on #682? (You could of course any time due the same I did and re-fork that branch from me to try to make further changes, it's probably just easier if we go forward from the "cleaner" single squashed rebased one.)

vorburger · 2020-01-11T00:26:57Z

Definitely closing this in favour of #682 (which is now green).

FINERACT-702: Integrate SpotBugs and fix over 300 high priority Rule …

0eab19c

…Violations This PR also serves as a base solution for FINERACT-712 and FINERACT-696 Review thoroughly

vorburger requested changes Dec 12, 2019

View reviewed changes

...ract-provider/src/main/java/org/apache/fineract/organisation/monetary/data/CurrencyData.java Outdated Show resolved Hide resolved

vorburger mentioned this pull request Dec 12, 2019

fix potential NPE in JobParameter.equals() (FINERACT-428) #672

Merged

awasum added 3 commits December 13, 2019 10:53

Use Objects.equals to prevent NPE and add check tasks to travis(runs …

e01249d

…spotbugs)

Merge branch 'develop' of https://github.com/apache/fineract into spo…

4787d9c

…tbugs

Fix spotbugs violations caused by new PRs

0409af3

vorburger mentioned this pull request Dec 14, 2019

Integrate SpotBugs and fix over 300 high priority Rule Violations (FINERACT-702) #673

Closed

vorburger requested changes Dec 14, 2019

View reviewed changes

awasum added 4 commits December 18, 2019 12:29

Address PR Review Comments

4524fbf

FINERACT-702: Integrate SpotBugs and fix over 300 high priority Rule …

3d54025

…Violations This PR also serves as a base solution for FINERACT-712 and FINERACT-696 Review thoroughly

Use Objects.equals to prevent NPE and add check tasks to travis(runs …

7f19a1c

…spotbugs)

Fix spotbugs violations caused by new PRs

6add7d5

awasum force-pushed the spotbugs branch from 0409af3 to 6add7d5 Compare December 18, 2019 11:36

awasum added 2 commits December 18, 2019 12:55

Merge branch 'spotbugs' of https://github.com/awasum/fineract into sp…

655d901

…otbugs

Address PR Review Comments

0468065

Merge commit '4524fbf' into spotbugs

awasum force-pushed the spotbugs branch 2 times, most recently from bbdf142 to 0468065 Compare December 18, 2019 13:01

Fix more spotbugs failures and integration tests

c66566c

awasum requested a review from vorburger December 22, 2019 14:52

awasum closed this Jan 5, 2020

awasum reopened this Jan 5, 2020

vorburger reviewed Jan 10, 2020

View reviewed changes

vorburger mentioned this pull request Jan 10, 2020

use SpotBugs and fix over 300 high priority Rule Violations (FINERACT-702) #682

Merged

vorburger requested changes Jan 10, 2020

View reviewed changes

vorburger closed this Jan 11, 2020

awasum mentioned this pull request Jan 14, 2020

FINERACT-614: Rates Module #592

Merged

6 tasks

Conversation

awasum commented Dec 10, 2019

Description

Checklist

Uh oh!

vorburger commented Dec 10, 2019

Uh oh!

Uh oh!

vorburger commented Dec 14, 2019

Uh oh!

awasum commented Dec 14, 2019

Uh oh!

vorburger 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!

Uh oh!

vorburger commented Dec 14, 2019

Uh oh!

awasum commented Dec 22, 2019

Uh oh!

awasum commented Dec 22, 2019

Uh oh!

vorburger commented Jan 5, 2020

Uh oh!

vorburger Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

vorburger commented Jan 10, 2020

Uh oh!

awasum commented Jan 10, 2020

Uh oh!

vorburger Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

vorburger commented Jan 10, 2020

Uh oh!

vorburger commented Jan 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants