Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement decision automation based on individual metrics #632

Merged
merged 72 commits into from
Oct 10, 2023

Conversation

Jeff-Thompson12
Copy link
Collaborator

Addreses #483

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #632 (3bfd0e1) into dev (829186d) will increase coverage by 0.76%.
Report is 1 commits behind head on dev.
The diff coverage is 80.47%.

❗ Current head 3bfd0e1 differs from pull request most recent head 389b17a. Consider uploading reports for the commit 389b17a to get more accurate results

@@            Coverage Diff             @@
##              dev     #632      +/-   ##
==========================================
+ Coverage   72.01%   72.78%   +0.76%     
==========================================
  Files          32       33       +1     
  Lines        4302     4710     +408     
==========================================
+ Hits         3098     3428     +330     
- Misses       1204     1282      +78     
Files Coverage Δ
R/app_config.R 100.00% <100.00%> (ø)
R/mod_databaseView.R 90.66% <100.00%> (ø)
R/mod_reweightView.R 95.67% <100.00%> (ø)
R/run_app.R 67.50% <100.00%> (ø)
R/utils.R 96.52% <100.00%> (+0.05%) ⬆️
R/utils_get_db.R 86.08% <ø> (ø)
R/utils_startup.R 63.95% <100.00%> (+0.21%) ⬆️
R/mod_addComment.R 96.77% <0.00%> (ø)
R/mod_communityMetrics.R 93.33% <0.00%> (ø)
R/mod_introJS.R 96.55% <0.00%> (ø)
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jeff-Thompson12
Copy link
Collaborator Author

Jeff-Thompson12 commented Aug 25, 2023

A number of items still need to be addressed.

  • Allow pre-configuration of the rules
  • Apply more rigorous checkers on the filters being supplied by users
  • Disabled submit buttons for incorrect values
  • Do we want to add color to easily identify decisions applied by rules?
  • Documentation
  • Tests

72DF8240-3AE3-4B53-9CC7-F4A47CF81501

@Jeff-Thompson12
Copy link
Collaborator Author

@AARON-CLARK I need to investigate why the test-coverage GHA is failing. It is failing in the reweight view test, so I don't think it is related to this PR. Plus the R-CMD-check is passing.

You should at least be able to check the changes you requested.

Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 PR. Once this is merged onto dev, I'd like to record a short 3 minute video for the pharmaverse's YouTube channel about this new feature. Can we collab on what points we'd want to make for it's debut?

Per our discussion offline, I think it would be prudent to try an implement an optional "else" decision category in case none of the rules are met!

@AARON-CLARK
Copy link
Collaborator

@Jeff-Thompson12, is this ready for another look?

@Jeff-Thompson12
Copy link
Collaborator Author

@AARON-CLARK It should be now.

Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Jeff-Thompson12, sorry to request changes. I know this one has been out there for a while!

Confirm ELSE

I just created a generic rule, with an else of "No Decision". Wondering if you could show that in the confirmation modal? It shouldn't exist as RULE 2 after clicking submit, but I think just acknowledging the selection here would be useful feedback.

image

Re-org

I think this interface is a little hard to follow visually, and could be tightened up with a few small changes. First, could you rename a few things in this section? Namely,

  • Automate Decisions by Metric Value or Risk Score --> Decision Automation
  • Rules List --> Decision Rules
  • Additional Details --> Details
  • Metric / Score --> Metric

image

Then perhaps a sub-header above the low, med, high checkboxes that says "By Package Risk Score". Try to make sure the new header is indented to match the current position of "Rule List", and the checkboxes could be slightly indented from the new sub header.

image

@Jeff-Thompson12
Copy link
Collaborator Author

@AARON-CLARK The changes you have requested have been implemented.

Copy link
Collaborator

@AARON-CLARK AARON-CLARK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥

@AARON-CLARK AARON-CLARK merged commit ac1c2c0 into dev Oct 10, 2023
3 checks passed
@Jeff-Thompson12 Jeff-Thompson12 deleted the jt-483-individual_metric_rules branch November 22, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants