DH-IT-Portal-Development / ethics

Ethical Committee web application in Django
http://fetc.hum.uu.nl
MIT License
2 stars 1 forks source link

Feature/4 small issues #579

Closed EdoStorm96 closed 1 year ago

EdoStorm96 commented 1 year ago

This PR fixes #350, #575 and #578.

EdoStorm96 commented 1 year ago

Ok, so I decided to also incorporate a fix for #491 here. I hope this PR is not getting too messy for you both ...

491: So this alters how the avg_understood field works. It is now nullable, the widget for it has changed to a radioselect button, and it can be either yes or no, but not be left blank. This field is now also displayed in the pdf & diff.

I did run into one issue here ... The AVGUnderstoodValidator function (the logic of which I updated in line with the field's new requirements) was seemingly not doing anything for me, so I tried removing it entirely. This seems to not be possible, when making migrations. So now it is just there, but not really doing anything? I created an error message in the form's clean method, which works fine, but could/should I remove the validator? Or is it best to keep it and get rid of my new error message? I did not really see the purpose of this validator ...

tymees commented 1 year ago

One last thing: you only added the is_committee_review variable. It would be nice if a app-wide pass was made to actually use it in QuerySets over the old annoying method

EdoStorm96 commented 1 year ago

Ok, so I implemented the changes you both requested, some notes:

EdoStorm96 commented 1 year ago

All right, so I've implemented Ty's minor tweaks, but, more importantly, I realized that it should be is_committee_review and not is_commission_review. This is a typo that occurred when discussing the original issue (#350) and was then adopted in this discussion and also by me ;p I re-ran the migrations on my branch, so that there is no evidence of this. I think it should be all good now!