OpenMRS Trunk

Attached find a patch as a replacement for the cohort-report-designer branch. This patch contains all necessary changes. To address the issues highlighted in the code review:

1. Context.setVolatileUserData() - Removed this altogether, as it is not currently used and needs more thinking through

1. DWRCohortService - Added comments as requested and removed use of "getCurrentEvaluationContext", instead substitution a new EvaluationContext.

1. CohortReportFormController - This was not addressed. This is a new web controller, which works as designed and will likely be replaced down the road. If refactoring is desired here, we can open a new ticket.

1. cohortReportForm.jsp - Fixed the tooltips to go back to default behavior since they were not working correctly. Will appear after short delay.

1. reportSchemaXmlList.jsp - Changed this slightly as requested.

1. User Interface (in general) - Added some instructive text to each Tab.

Patch attached.

Given that this has been code reviewed once, and touches very little existing code, I will plan to check this into trunk tomorrow pending comments.

I'm satisfied with the wording additions to the jsp form, thanks.

I don't remember saying we should get rid of the getCurrentEvaluationContext(). I think we wanted to change it from getMyEvaluationContext() (which I did, to make it "current")

Thanks Ben, We can certainly tweak this wording going forwards. See my comment on the other ticket regarding the "getMyEvaluationContext" - it just isn't currently needed, and I didn't feel it should be added if it only adds unnecessary complexity.

Which ticket do you mean?

Other comment: wasn't the cohort builder using the setVolatileUserData() method for "temporary" storage of the users search history?

I mean ticket TRAC-1146: ReportService - Initial Refactor

Yes, the setVolatileUserData() still exists, and is presumably still used by the cohort builder. It is no longer used by this patch / branch (since I took out the logic around "getMyEvaluationContext() / getCurrentEvaluationContext()". So, it should not be an issue that needs resolution for this to be merged into trunk.

Ah, didn't realize you refactored to take out the need for getCurrentEvalContext. Good work.

Changes committed to trunk in changeset rev:6193.

Final changes attached as patch cohortReportDesigner.2.patch

Code review document updated to reflect fixes. One new ticket raised - TRAC-1150 - for some additional refactoring work.