Refactored Nag Dialogs, Split UnableToAffordExpensesNagDialog into Two Dialogs, Added Unit Tests

[IllianiCBT]

Refactored a majority of our existing nag dialogs to standardize the code used within.

Modified the UnableToAffordExpensesNagDialog to specify that only monthly expenses are being checked. Updated the Java method to exclude loan data.

Added a new nag that triggers specifically if the campaign is unable to afford any loan payments due the next day.

Moved the StratCon outstanding contact nag logic into the relevant nag dialog class.

Added Unit Tests for every nag dialog, barring the single dialog that already had Unit Tests.

Closes #4834

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.45%. Comparing base (752cc96) to head (1ea5472).
Report is 70 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4837      +/-   ##
============================================
+ Coverage     10.30%   10.45%   +0.14%     
- Complexity     5871     5957      +86     
============================================
  Files           944      945       +1     
  Lines        131394   131538     +144     
  Branches      19063    19079      +16     
============================================
+ Hits          13545    13748     +203     
+ Misses       116532   116466      -66     
- Partials       1317     1324       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[IllianiCBT]

Flipping to draft while I write some Unit Tests

[IllianiCBT]

Moved these functions into their own discrete methods, as it made sense to isolate them in case we ever want to move them into their own class (like the other nag dialogs).

[Sleet01]

Just an FYI, not requesting any changes:

Private members of a class are only visible to instances of that class, so will not be available for unit testing.
It is possible to write less-protected wrapper functions that then call the private methods just for testing purposes - that is, they can then be called in unit tests, but the private methods themselves will be invisible to unit testing code.

For that reason, it is common to see code that has unit tests get switched from private to protected - the Junit unit test code needs at least this much access to call the methods.
Package-private is another option iff the unit tests can be placed in the package hierarchy of the code under test, but I don't think we have that option with Junit 5.

[Sleet01]

This is all really good! I have a single recommendation, not necessary to implement here but something to think about going forward:

Check out "parameterization", a method for reducing copy-and-pasting a bunch of similar tests (q.v. https://www.baeldung.com/parameterized-tests-junit-5).
When you have a set of tests that are identical except for some parameters that they consume, you can have Junit run the same test function with different combinations of parameters, thusly:

@ParameterizedTest
@ValueSource(ints = {1, 3, 5, -3, 15, Integer.MAX_VALUE})
void isOdd_ShouldReturnTrueForOddNumbers(int number) {
    assertTrue(Numbers.isOdd(number));
}

So this allows for running a test function multiple times with different inputs, cutting down on repetitive code.
There are drawbacks, though:

One of the limitations of value sources is that they only support these types:
short (with the shorts attribute)
byte (bytes attribute)
int (ints attribute)
long (longs attribute)
float (floats attribute)
double (doubles attribute)
char (chars attribute)
java.lang.String (strings attribute)
java.lang.Class (classes attribute)
Also, we can only pass one argument to the test method each time.

Other things to note:
This only works if the assert is the same for every combo of parameters or if you can pass in the expected value as an additional parameter... which is usually too much hassle.
So in this case, you could parameterize all the assertTrue tests into one function, and all the assertFalse tests into another, if initializeUnits() could be set up to take a single value rather than 4.

Again, not requesting any changes here, just something you might want to look at for future work.

[IllianiCBT]

This is very cool, thank you for the reading suggestion. I might be able to get it to work here if I can work out an algorithm that will allow us to test all the possible combinations, but I think in this instance what we would gain in brevity we'd lose in readability.

I know you said you weren't suggesting change, but I wanted to comment my thought process.

[Sleet01]

Looks very good, outside of already-identified changes and one minor typo.

[IllianiCBT]

Flipping this to 'live'. Description and title have been updated to reflect changes.

@Sleet01 there have been a bunch more Unit Tests added since your earlier review. I don't know if you want to take another look before confirming it has your seal of approval.

[rjhancock]

Other than the JavaDoc stuff I mentioned, (yes I know, minor stuff but we all need to get better at it), it looks good to me.

[IllianiCBT]

That should all be handled now - was there one I missed?

[rjhancock]

Half a dozen or so. I tagged them in the review changes section.

[IllianiCBT]

I believe I've caught the last remaining JavaDoc issues.

[rjhancock]

To help find the issues, run ./gradlew javadoc and it'll output a report of warnings and errors. Errors MUST be fixed, warnings RELATED to your code SHOULD be but NOT required.

[IllianiCBT]

That command is invaluable, thank you

[rjhancock]

I'll be adding that to the Actions soon as part of a requirement of things passing. And eventually doing one for GH Pages of JavaDoc so be mindful and descriptive of your documentation. Write it for people who DON'T code.

[IllianiCBT]

Sounds good. I've been getting into the habit of writing JavaDocs for just about everything.