-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
WB-918 add new state mergereview, filter by oldest day with things in… #72
Conversation
… all enconter states except finished, filter per site locationID if present in both match candidate and focal encounter. WB-914 create Decision.updateEncounterState and submethods, automatically call updateEncounterState in Decision store upon a new Decision creation. Set min number of decisions needed and min number of merge decisions for a candidate match in commonConfig and use its logic in updateEncounterState to set state to mergereview or disputed as needed, and enforce the logic for showing volunteers the encounters based on these threshold numbers. WB-1370 add disputed state to queue. WB-909 add column in the queue for encounter level of agreement called Level of Agreement. WB-917 make queue show encounters in various states on an oldest-day-requiring-attention basis.
Query query = pm.newQuery(queryString); | ||
query.setClass(Decision.class); | ||
List<Decision> decisions = (List<Decision>) query.execute(); | ||
if (decisions!=null && decisions.size()>0) return decisions; |
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.
not sure how "bad" it is to return here before you do the query.closeAll()
... but i think these should be flipped around here?
myShepherd.rollbackDBTransaction(); | ||
} | ||
}else{ | ||
if(numberOfAgreementsForMostAgreedUponMatch < MIN_AGREEMENTS_TO_CHANGE_ENC_STATE){ |
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 think this if() {
block may be unnecessary, if i am keeping track of all my nestings correctly! i believe the else
right above it is tied to if(numberOfAgreementsForMostAgreedUponMatch >= MIN_AGREEMENTS_TO_CHANGE_ENC_STATE)
above... which means (i think!) this if
is the only case we could be in here.... does that make sense? i dont think this affect functionality but may just be adding an unnecessary complexity.
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.
Agreed completely. Just wanted to be super explicit about the case in which I wanted the disputed state to happen for readability.
@@ -69,6 +70,7 @@ public void doPost(final HttpServletRequest request, final HttpServletResponse r | |||
Decision dec = new Decision(user, enc, key, val); | |||
myShepherd.getPM().makePersistent(dec); | |||
ids.put(dec.getId()); | |||
Decision.updateEncounterStateBasedOnDecision(myShepherd, enc); |
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 might argue that this updateEncounterState...()
could be moved to outside (below) the for ( ... multiple.keySet) {
loop. maybe. haha. i am fine with it being here for multiple reasons (not the least of which is that i am not sure if multiple-mode is even used any more!) but just making a note of it here, in case later there seems to be some kind of slowdown for this being calculated once per loop... when i think it might be sufficient to alter encounter.state after all decisions have been recorded. note to future hopefully-no-one!!
8648f3a
to
5bcebf7
Compare
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.
some minor notes, only for prosperity -- dont think they will change functionality. looks good to me.
oops... i guess i would say that maybe that return-before-closeAll should be fixed in Shepherd.java first? |
…r code review feedback
… all enconter states except finished, filter per site locationID if present in both match candidate and focal encounter. WB-914 create Decision.updateEncounterState and submethods, automatically call updateEncounterState in Decision store upon a new Decision creation. Set min number of decisions needed and min number of merge decisions for a candidate match in commonConfig and use its logic in updateEncounterState to set state to mergereview or disputed as needed, and enforce the logic for showing volunteers the encounters based on these threshold numbers. WB-1370 add disputed state to queue. WB-909 add column in the queue for encounter level of agreement called Level of Agreement. WB-917 make queue show encounters in various states on an oldest-day-requiring-attention basis.