OpenClinica / enketo-oc

OpenClinica's fork of the Enketo web forms monorepo
Apache License 2.0
0 stars 2 forks source link

multiple constraints #176

Open MartijnR opened 6 years ago

MartijnR commented 6 years ago

Avoiding multiple constraint attributes

MartijnR commented 6 years ago

This feature is really about being able to show different constraintMessages.

pbowen-oc commented 5 years ago

We have some ideas about how to accomplished the primary goal of this (showing distinct messages for distinct error conditions) without literally using multiple constraints. We are thinking that we may be able to show the additional messages (i.e., constraints 2+) via separate note items after the the primary input item, but we would want these to be tied to the query status on the primary input item. Let's discuss on our next call to see if this approach seems workable in general. I think it would at least avoid the performance concerns you had about supporting an arbitrary number of constraints per item.

On our end, I think we could handle the automatic creation of the additional note items based on custom columns in the XLS spreadsheet. For a complete solution, I think what we'd need is a way to mark these notes so that Enketo could identify them and act appropriately (i.e., as if a constraint is firing) when the user tries to Close, Complete, or Navigate on the form while one of these notes is currently relevant.

Basically, I'm thinking that we'd have the user define a second constraint using columns bind::oc:constraint2, bind::oc:constraint2_message, and (eventually) bind::oc:constraint2_type. We'd automatically convert that into a note item in the XForm that has Relevant = not(constraint2) and Label = constraint2_message. We'd need Enketo to consider that item being relevant as the same thing as a constraint firing on it's parent input item.

MartijnR commented 5 years ago

Another consideration is auto-closing queries.

It seems to be better to re-design the discrepancy note functionality by taking out the comment-status() checks from the form logic and built it into the dn functionality itself.

As a consequence the discrepancy note functionality will have to become more intelligent and start monitoring the state of the question it links to. Depending on how we implement multiple constraints, it may have to be linked to multiple questions (with bind::oc:for) and monitor all of them.

MartijnR commented 5 years ago
MartijnR commented 5 years ago

considering adding constraint to dn question and display constraint error in parent question

MartijnR commented 5 years ago

Look into bind::oc:constraint2 etc (unlimited) with bind::oc:constraint2_message as it seems it might actually be easiest and fastest.

MartijnR commented 5 years ago

http://localhost:8005/single/fs/i/ruEJ8fPv?ecid=1

Things to solve:

MartijnR commented 5 years ago

@pbowen-oc, I'm realizing it's quite important to figure out how this will (or may in the future) play with the strict validation of constraints. It would be great if strict validation only optionally applies to the master constraint (i.e. the "ODK constraint", if constraint-type is set to 'strict'), and that the additional "OC constraints" are all the same (i.e. all non-strict or all strict). Does that seem like it would work for OC?

pbowen-oc commented 5 years ago

@MartijnR - I'm not certain, but I suspect we would be fine limiting it to a single strict required and/or a single strict constraint per item with all other constraints on the item being non-strict. I don't think it would work if having a strict constrain for an item meant that all other constraints on that item had to also be strict.

pbowen-oc commented 5 years ago

We need to consider how this interacts with multiple queries. Ideally, we want there to be an association between a query thread and a constraint, but this may be extremely complex.

pbowen-oc commented 5 years ago

@MartijnR - Can you estimate how much effort it would take to implement each phase of what we had discussed earlier? a) multiple constraints (where any query suppresses all constraints) b) tie each constraint to a query thread, have each constraint only be suppressed by the query thread associated with it, have autoqueries added to the appropriate thread for a given constraint, and allow the user to add queries either to an ad hoc thread or a constraint thread (possibly via some way to select a constraint/ad hoc when adding a new query)

MartijnR commented 5 years ago

a) 40-80 hours b) I cannot estimate this with any reasonable accuracy, other than it will at least double the work of a.

pbowen-oc commented 4 years ago

We are ready to go ahead with the first part of this after Anonymous mode is implemented. As you work on it, please keep in mind that we will need it to be connected to our query thread model for our eventual release.

pbowen-oc commented 4 years ago

@MartijnR - It has been a long time since we discussed requirements for this. Let's discuss to make sure are on the same page regarding how multiple constraints would be evaluated and errors would be displayed.

MartijnR commented 4 years ago
MartijnR commented 4 years ago

http://localhost:8005/single/fs/i/MjMXeuoj?ecid=1 (Centro)

MartijnR commented 4 years ago

@pbowen-oc, I think this was asked and answered somewhere, but I cannot find it. Does a maximum of 20 additional constraints per question seem reasonable?

pbowen-oc commented 4 years ago

@MartijnR - Yes, I think that will handle all reasonable cases with ease. Most cases will likely only use a few extra constraints.

MartijnR commented 4 years ago

@pbowen-oc There is some conflicting info in my notes. I'm thinking you'd like to only show one constraint error at a time.

If so, we would evaluate constraint, oc:constraint1, oc:constraint2, ... etc in that order, until one of them fails and then show the error message corresponding to the failed constraint (with a fallback if message if missing).

Is this correct?

pbowen-oc commented 4 years ago

@MartijnR - I'm looking into it further, but I think we would want to see all messages so that the user knows all ways in which their input is invalid.My assumption is that we would similarly want autoqueries to be added for each failed constraint if the user closes the form without querying the item first.

Also, I'm thinking that this would tie in better to future desired behavior when we can associate each constraint with a query thread.

MartijnR commented 4 years ago

Ok, thanks. I'll await for confirmation. I think what you mentioned would actually be easier to implement.

pbowen-oc commented 4 years ago

@MartijnR - Confirmed, we would like to show messages for all failed constraints.

MartijnR commented 4 years ago

I did't see any code that deals with this with default constraint, so I left as is for now Copied the same logic to remove custom constraints

pbowen-oc commented 4 years ago

We need to define the behavior when multiple constraints are active and 1) the user goes into the Query Widget to add a new query and 2) the user Closes the form and autoqueries are added.

MartijnR commented 4 years ago

This feature is now ready to be merged, tested and built-upon (for now it's in a separate branch). Known things todo are:

MartijnR commented 4 years ago
MartijnR commented 4 years ago
pbowen-oc commented 4 years ago

To do on OC end:

pbowen-oc commented 4 years ago

Another case that we thought of: If a strict constraint is firing (whether or not any non-strict constraints are firing), we would want the background for the item to be the orange strict error background. If there are no strict errors present but there are non-strict errors present, then the background should be the pink background for non-strict errors.

And one more question: Will participate be able to handle additional constraints all as strict constraints that don't allow the user to continue navigating forward?

MartijnR commented 4 years ago

Another case that we thought of: If a strict constraint is firing (whether or not any non-strict constraints are firing), we would want the background for the item to be the orange strict error background. If there are no strict errors present but there are non-strict errors present, then the background should be the pink background for non-strict errors.

created issue: https://github.com/OpenClinica/enketo-oc/issues/143

And one more question: Will participate be able to handle additional constraints all as strict constraints that don't allow the user to continue navigating forward?

Probably, with some complicated and difficult-to-maintain code changes. Please create a new issue if needed.

pbowen-oc commented 4 years ago

@pbowen-oc - Check if Participate automatically marks each constraint and required as strict. If it does and that is how the feature currently works there, consider only supporting one constraint in Participate.

MartijnR commented 3 years ago
MartijnR commented 5 months ago

@pbowen-oc: are multiple constraints still a desired feature? If not, it will save time to remove the custom code instead of moving it to enketo-core.

MartijnR commented 5 months ago

FYI, I believe this is the test form I was using: https://docs.google.com/spreadsheets/d/1Ws_Dgl_EjyDLa5f_xInOVAYp5Kt6LQAgbPRpSdEq6M0/edit?usp=sharing