Code smells iteration 3.1

This PR resolves code smells in the file ViewDiagnosisStatisticsAction.java. It includes the following changes:

• Removed unused imports
• Replaced magic numbers with references to constants
• Shortened long methods, and extracted repeated code into helper functions (DRY)
• Simplified complex boolean expression by extracting components to local variables
• Shortened lines longer than 100 chars
• cleaned up stale comments

marked this merge request as draft

changed the description

closed

reopened

marked this merge request as ready

changed the description

assigned to @htmoss2 and unassigned @adityab3

These changes cause more tests to fail than the previous 15, this causes 23. This might also be from the previous merges, I am checking into that now.

What did you run to check this? Also did you merge to master locally?

I'm using intellij then right clicking on the tests folder, and clicking run all tests. I'm trying to find a way to do this on command line for all you doing this on the VM, I'm running ubuntu on my PC. I did not merge to master locally, I made a different branch locally and then git pulled

Got it, could you let me know which test are failing? This PR only changes one file, and that file doesn't have so many references, so it seems odd that it would cause that many tests to fail.

Ok, might be the other UC that just went through, or the other code smells... On a previous commit I'm getting 22 failed already and I'm only halfway through testing.

Let me try merging locally and see what tests fail.

22 have failed from this previous revision, so you must have 1 test failure...

Let me see if I can get this command line command to run all 2345 tests. Give me 10 mins.

Edit: Heh some of this might be on me I forgot to mvn install XD

Haha I've done that a lot. Still waiting on my tests to finish too

I tried merging it into master locally and running, I get SearchUsersActionTest.testDeactivated:147 expected:<2> but was:<1> as my only error. Let's see if anyone can replicate this.

Yep I'm getting that and 14 selenium failures for the "Merge branch 'UC14' into master'" I think it must've been on my end, but let me test your commit now

I just finished testing it on the code_smells branch and got 12 errors:

AppointmentTest.testViewApptWithConflicts:163 » NoSuchElement No link found wi... AppointmentTypeTest.testPatientViewUpcomingAppointments:134 » NoSuchElement No... ViewTransactionLogsTest.testAdminImpossibleDate:271 » NoSuchElement No link fo... ViewTransactionLogsTest.testAdminIncorrectDate:236 » NoSuchElement No link fou... ViewTransactionLogsTest.testAdminLogin:27 » NoSuchElement No link found with t... ViewTransactionLogsTest.testAdminNoResults:305 » NoSuchElement No link found w... ViewTransactionLogsTest.testAdminRegular:45 » NoSuchElement No link found with... ViewTransactionLogsTest.testAdminSelectMainRole:113 » NoSuchElement No link fo... ViewTransactionLogsTest.testAdminSelectSecondaryRole:155 » NoSuchElement No li... ViewTransactionLogsTest.testAdminSelectType:197 » NoSuchElement No link found ... ViewTransactionLogsTest.testTesterLogin:36 » NoSuchElement No link found with ... ViewTransactionLogsTest.testTesterRegular:79 » NoSuchElement No link found wit...

@htmoss2 interesting... I'm getting the ones that I posted on Slack for the most part, and am running this branches tests rn, we shall see the results in a few mins

Okay I just ran it on a local version of master with the new code smells and just got

Failed tests: SearchUsersActionTest.testDeactivated:147 expected:<2> but was:<1> Tests run: 1795, Failures: 1, Errors: 0, Skipped: 0

Does that mean it got through all the tests though or just stopped at the first one?

I get a nullptrexception for DiagnosisStatisticsBeanTest

It likely got through all the tests (I am showing 1799 unit tests) but not the selenium tests (there are like 700)

We fixed that one in !28 (merged)

Nice, I'm seeing that too. So we have all good tests on this branch. Approving the merge.

I see the same amount as on master, and we've had 15 total failing for at least half the day.

approved this merge request

Just confirming that this is good to merge

Confirm.

merged

mentioned in commit ec7e9881