Feature/revise study end questions #625
Conversation
There was a problem hiding this comment.
My main comment with this PR is the model to which these fields belong. Although Desiree mentioned in the issue that she wanted them here, I'm not fully certain she knows that the question will be asked for every single trajectory.
I would consider the proposal model to be a more logical place, near the ethical considerations question as initially suggested. Definitely when it comes to researcher safety, I don't think those kinds of issues every relate to just one trajectory. If it's ever a yes, it's probably for the entire proposal.
studies/models.py
Outdated
| door het onderzoek." | ||
| ) | ||
| "Bevat bovenstaand onderzoekstraject elementen die <em>tijdens\ | ||
| </em> de deelname zodanig belastend zijn dat deze vragen, \ |
There was a problem hiding this comment.
I don't know if this is a conscious change, or something that Black took liberties with. But this new whitespace is actually significant.
A line with a trailing \ is continued in a very low-level manner, without the sanity checks Python usually provides. The interpreter literally sees the next line as one with the previous line, except really long. Therefore the leading indentation whitespace gets included in the string. That's why these msgstrs have loads of whitespace in django.po now.
Somehow gettext doesn't mind (they're not fuzzy or anything) but this looks pretty awful:
The recommended way to continue strings is to just do it implicitly:
negativity = models.CharField(
_(
"Bevat bovenstaand onderzoekstraject elementen die <em>tijdens"
"</em> de deelname zodanig belastend zijn dat deze vragen, "
"weerstand, of zelfs verontwaardiging zouden kunnen oproepen, "
"bijvoorbeeld bij collega-onderzoekers, bij de deelnemers zelf, "
"of bij ouders of andere vertegenwoordigers?"
),
max_length=1,
choices=YesNoDoubt.choices,
blank=True,
)Just keep adding strings without commas inbetween and python will implicitly join them. Or use triple quotes, str.join(), or literally anything other than backslashes, lol.
A lot of the strings here were already continued with backslashes from ages ago, which might not have been such bad practice back then. I wouldn't ask you to fix all of them just try to keep the strings you change free of unnecessary whitespace.
reviews/utils/review_utils.py
Outdated
| if study.researcher_risk in [YesNoDoubt.YES, YesNoDoubt.DOUBT]: | ||
| reasons.append( | ||
| _( | ||
| "De onderzoeker geeft aan dat er mogelijk kwesties zijn \ |
There was a problem hiding this comment.
If we end up keeping these fields in the study model, it might be nice to add the trajectory number in this string.
f"De onderzoeker geeft aan dat er mogelijk kwesties zijn rondom de veiligheid van de betrokken onderzoekers in traject {{study.order}}."
Right now, if a proposal has two trajectories and both have trajectory risk, this reason will just show up twice identically.
Also please fix the backslash whitespace issue here as well.
Removed researcher_risk and knowledge_security fields from the Study model, as these are going to be moved to the Proposal model.
|
So, I pretty much implemented your requests, Michael. However, I've discussed with Ty that this PR is a bit redundant right now, with the big makeover coming up. I just wanted to finish this implementation, as documentation, but there is no real need to review it as we are most likely going to redesign this from scratch when changing to DSC 3. |
Fixes #546. Mostly textual changes, but also added some new fields on study_end. These changes were made according to Desiree's requests which she wrote up in a e-mail. A lot of text, but nothing too crazy functionality-wise.