Dealing with deleted proposals the right way

[gavia]

@thewheat @tmccarthy

The current build implements a really crappy way of the concept proposer dealing with proposals that have been deleted by the dictionary manager (AKA the concept reviewer).

On the API side, the concept proposer requests an updated list of all proposals and their details from the reviewer. They then cross reference this list with the local list of proposals, and if any have been deleted (that is, if any proposals exist in the local list but do not exist in the list retrieved from the reviewer), the status of said proposal is changed to DELETED, which essentially means read-only.

This is crappy for a number of reasons, primarily being that there is little error checking. If either the concept reviewer or concept proposer is offline at the time of refresh, instead of recognising this and returning an appropriate error, the concept proposer sees a null list and assumes all proposals have been deleted. This issue is later rectified once connection is made again but that's besides the point.

What really needs to happen is that each time a refresh occurs, the proposer needs to first check if connection can be made to the reviewer. If connection can be made successfully, this then guarantees that a null list is actually a list of no proposals. If not, then the refresh request can return an appropriate error message (such as no connection can be made, please check your settings).

Looking into this further, there is code existing in concept propose under omod/src/main/java/org/openmrs/module/conceptpropose/web/controller/CpmSettingsController that does just this. The public method testConnection will return Success if connection can be made, or some error message otherwise.

So how do I use testConnection inside of ProposalController, and what the hell is all this @ResponseBody stuff??

[gavia]

Oh, and my idea from last night about not really ever deleting a proposal on the concept reviewer side is, I think, a bad one. This has the potential to blow up the size of the reviewer's database very quickly, and also transmit essentially irrelevant data for each refresh. I think that deletion needs to be something that is dealt with on the proposer side.

[thewheat]

• I think the response from the proposer needs to more stringent: a response from the dictionary manager should be such that we know we have received a valid response
• If there is no connection, the proposer side could throw an exception / error which can be thus handled appropriately (i.e. should not return an empty list which marks things as deleted on the proposer side)
• If things are handled based on what I mentioned above, testConnection doesn't need to be used (also need to consider the fact that network could get disrupted after checking if server is up). However, perhaps the core testConnection functionality to test connectivity could be moved to a common class to be reused in other places should the need be.
• Personally I would prefer double deletion on the review side (first makes it a 'DELETED' status, second deletion, deletes from DB). I don't like the fact that a possible single accidental deletion could mean that data is lost without possible recovery. Proposer could submit a list of UUIDs of their proposals and the review could respond appropriately: Closed/Opened/Deleted/Doesn't Exist (new doesn't exist status?).
• I think @responsebody is a Spring thing to format the class in the appropriate format for a web response

[thewheat]

If I don't make it tonight, I've done a bit of work for this change https://github.com/thewheat/openmrs-cpm/tree/refresh-proposal-status