Properly handle zero regions selected in review configuration

[jthompson-arcus]

Fixes #35

Selecting zero regions will now update the site picker input. Selection zero sites will also result in user feedback. Below is a snapshot of the new behavior.

[jthompson-arcus]

A test has also been added for this situation.

[LDSamson]

@jthompson-arcus Thanks for the PR, it works well! I added a suggestion for a minor improvement of the added test. For the rest it is all good I think.

[jthompson-arcus]

I did end up wrapping the test in test_that(). Main thing is that the environment within that function is different. You have to use clinsight:::function() to access the modules.

I think maybe I should switch it back though because it is currently registering a skipped test.

[LDSamson]

@jthompson-arcus two final comments, then I'd be fine if you finalize this: maybe remove both the clinsight::: expressions as well now that it is not needed anymore. And the version number can be bumped again since I just merged Aarons PR with version number 9005 with the dev branch. Have a nice weekend!

[jthompson-arcus]

Unless they are causing harm in some way I would prefer to leave the clinsight:::function() calls. Otherwise you cannot run the code line by line when building or debugging the test.

[LDSamson]

@jthompson-arcus two final comments, then I'd be fine if you finalize this: maybe remove both the clinsight::: expressions as well now that it is not needed anymore. And the version number can be bumped again since I just merged Aarons PR with version number 9005 with the dev branch. Have a nice weekend!

Sorry I just saw you left the clinsight::: expressions intentionally. I am not a fan of the notation, but provided that it does not give problems with renv when you reload the package (haven't got the time to test it), I would be okay to leave it in, if it is easier for you to run the tests. In the future, it would be nice to find a better solution for this though.

[jthompson-arcus]

I see. Would you be okay if I just add clinsight to the list of ignored packages for renv?

[jthompson-arcus]

In the future, it would be nice to find a better solution for this though.

I'm personally actually curious how your tests were working without using it. I thought it was the only way to write "dummy" app tests for individual modules. If the code was encased in test_that(), I don't think you can get the test working without using clinsight:::function().

[jthompson-arcus]

I have decided to remove the ::: calls. While I think in this use case they are fine since we would be referencing our own package, it is considered bad practice. This did send me down a little bit of a rabbit hole though and I found the following article that I thought was an interesting and informative read: https://www.tidyverse.org/blog/2022/09/playing-on-the-same-team-as-your-dependecy/.

[LDSamson]

In the future, it would be nice to find a better solution for this though.

I'm personally actually curious how your tests were working without using it. I thought it was the only way to write "dummy" app tests for individual modules. If the code was encased in test_that(), I don't think you can get the test working without using clinsight:::function().

We can have a short meeting about it this week if you want. I am also curious about the problems you are encountering.
The process I try to follow for module tests is as described here.