-
Notifications
You must be signed in to change notification settings - Fork 11
[TTAHUB-4152] Consider Findings done after Monitoring Goal closure #3171
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
[TTAHUB-4152] Consider Findings done after Monitoring Goal closure #3171
Conversation
kryswisnaskas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update to the monitoring goal logic and documentation!
A quick check on grants_needing_goal. Shouldn't we also link the open_citations in the logic there?
Whoooops! Somewhere along the way there was a significant reversion. I'll fix that and repush |
kryswisnaskas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
|
The changes on this are pretty large so they need rereview |
| ), | ||
| -- Select all the potentially-relevant reviews | ||
| -- for early filtering of monitoring datasets | ||
| all_reviews AS ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't used later, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugggh it isn't but it probably should have been in ordered_citation_reviews. I changed the citations ordered_citation_reviews to match what was in createMonitoringGoals but using all_reviews would be faster. Trying to keep these things consistent is a giant PITA and I can't wait to replace most of this with the fact tables. It's probably fast enough this way but it would be faster if I used the more refined version here which is closer to what's in the recipient spotlight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly defer to your expertise, but if it's just a performance issue, the query ran pretty quick on prod data, so if it's accurate both ways it might be good to just wait for the fact
thewatermethod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the script, runs without error. Data modifications look to be correct.
Co-authored-by: thewatermethod <[email protected]>
Description of change
This now updates both the citations service and the Monitoring goal creation process to follow all the additional logic we know about:
MonitoringFindingHistory.determinationfor the latest review as the findingType, unless null, in which case it defaults back toMonitoringFindings.findingTypeThis is considerably more comprehensive than the original Jira/PR so needs to be re-reviewed.
How to test
This is not an easy one to test separately. On way to see it working because the AOC ED80AFF2-78CF-40C0-BBC4-9C5258DC22C8 isn't present in the
open_citationsCTE and so won't trigger a Goal opening.I would expect it to open goals for:
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
After merge/deploy