Feature/all attachment kinds #753
Conversation
…ure/all-attachment-kinds
There was a problem hiding this comment.
Awesome! I'm very happy with this.
At first I thought that putting everything in one checker would make it messy, but this works fine.
Please take a look at #755 for making it more clear when only one of a set of documents is required.
There are however some issues with the logic. Here's the first one:
Als op dit punt bij de grondslag toestemming nog geen toestemmingsverklaring (voor kinderen) verplicht is, wordt de gewone toestemmingsverklaring grondslag toestemming verplicht gesteld.
I believe that in the current state, that final "toestemmingsverklaring grondslag toestemming" is made mandatory even if the consent forms for kids are already mandatory. That shouldn't be the case.
I believe the same should be the case here:
In het geval dat er in het traject is aangegeven dat er bijzondere persoonsgegevens worden verzameld, en er nog geen akkoordverklaring voor opnames of script voor het maken van opnames in ingediend, wordt bij algemeen belang de toestemmingsverklaring algemeen belang met bijzondere persoonsgegevens verplicht gesteld.
Although here it's definitely an error of mine in the "law". I would reword the above as follows:
In het geval dat er in het traject is aangegeven dat er bijzondere persoonsgegevens worden verzameld, en er nog geen akkoordverklaring voor opnames of script voor het maken van opnames verplicht is gesteld, wordt bij algemeen belang de toestemmingsverklaring algemeen belang met bijzondere persoonsgegevens verplicht gesteld.
Basically this avoids the case where an attachment is mandatory only if a different mandatory file has not yet been added. That makes no sense, because that means that the first file will always be optional, because the second file will always be added anyway.
I think this makes the logic simpler in the end, but sorry for the extra work cause by my mistake.
proposals/utils/checkers.py
Outdated
| fullfilled_slot = None | ||
| for slot in slots: | ||
| # check if any of these slots have been fullfilled yet using match() | ||
| slot.match(exclude=[]) |
There was a problem hiding this comment.
This is somewhat abusive of the match function, because slot.match() modifies the internal state of the slot, setting its kind and attachment based on what it finds.
I think this currently does no tangible harm, but it's definitely something that can potentially go wrong in the future. I would like to request the following change:
- Modify
slot.match()to just return the matched attachment or False if nothing is found. It should no longer change the state of the slot. - Add a new
slot.match_and_set()method that callsmatch(), and if it finds something, it setsself.kindandself.attachment.
Like so we can use slot.match() for use cases like in this checker, and use match_and_set() where we actually desire to change the state of the slot, such as in stepper.add_slot() and stepper.attachment_slots().
Otherwise reusing the match method here is a great solution, I just don't like all these slots getting different attachments and kinds set for every pass of the checker.
|
I would say I have adressed all you comments. I have also made it the case, that a consentform for adults gets required if the study has adults and the stuff for children, if a study has children. These being interdependant did not make much sense, although we will check with Desiree on this. |
attachments/utils.py
Outdated
| # If we have fewer than two members, we just append | ||
| # the members. Addition allows for the empty list edge | ||
| # case to work. | ||
| out += item.members |
There was a problem hiding this comment.
This is an error by me. I wrote this before turning members into a set(). But we can't add sets and lists together.
| matched_attachment = self.match(exclude=exclude) | ||
| if matched_attachment: | ||
| self.attachment = matched_attachment | ||
| self.kind = get_kind_from_str(matched_attachment.kind) |
There was a problem hiding this comment.
Match_and_set should actually still return True, or the instance, or anything truthy if it succeeds. Otherwise stepper.attachment_slots() will not work.
Although in that specific method we do actually want to modify internal slot state.
| @@ -66,7 +66,7 @@ def attachment_slots( | |||
| obj, | |||
| force_desiredness=desiredness.EXTRA, | |||
| ) | |||
| success = empty_slot.match(exclude=exclude) | |||
| success = empty_slot.match_and_set(exclude=exclude) | |||
There was a problem hiding this comment.
This is why match_and_set() needs to return something. Its return value matters
Woopee, there we are. While closely reading along the master instruction and this page I've implemented all the possible attachment kinds into the new attachment system. While writing up the different kinds was doable ... the logic for when certain slots are to be added with the right desiredness was quite a pain and led me down deep paths of the portal's architecture. The bulk of the action happens in the
StudyAttachmentChecker. I am not sure if this is the most elegant solution for implementing all this logic ... But it becoming somewhat of a mess was unavoidable ... But I'm open to tips for improvements! I think the best way to understand all this is to keep the document/email with the master instruction close at hand.Checking whether a study features recordings is especially annoying due to #733... I was also struggling a bit with making sure the other of a pair becomes optional when one is present. The wishes for this are also quite specific, so I could not find a nice way to generalize this behavior.
I added some pretty extensive descriptions taken from the page linked to above. These are currently not used, but might come in handy later.
I though I would also tackle translations, so I did this for the whole attachments branch.
Finally I implemented the attachments page into the general flow of the form, so now the back and forward buttons work correctly ... As of the current state of the portal this requires a form, so I added an empty form for enabling this.
Sorry for the big commits, (6654c4a & ba2ddfd) but it just made most sense as a whole imo.