SAML to prod #530
Merged
SAML to prod #530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oh come on Ty, you're normally the one constantly bugging everyone else about this and now you're guilty of doing it!
It can handle non-SAML sessions as well
* fix: prevent discontinueing of accepted proposals * update successurls except supervisor revision * remove redundant lines * update secretary email hyperlink for METC doubts * update mechanical turk recruitment * TODO finish help texts for registration * make student_program conditional * add student_program to forms and models * add migrations * add student_program to proposal pdfs * add translations for new fields and help texts * tidy up migrations * remove temporary help text * add student_context field and migrate * add documentation check_field_required * add student_context_details field * cleanup translation * add helptext for registrations * migration for helptext registration * cleanup migrations * update helptext registrations * add translations helptext registrations * update project funding detail question * update fixture description * add gettext to student_context_details * update help text registrations * migration and translation * update translations * make student_context and _program required * Create issues.yml * Fixed incorrect version * fix inaccuracy w requirement of student questions * add has_traits help text * update help text translation * compile messages * add student_justification field * add translations student_justification field * add helptext for date_start * add translations helptext date_start * add fields for hierarchy and hierarchy_details * add translations for hierarchy and details * add fields for special details * fix inaccuracy with dependency check has_traits * change traits dependency to medical_traits * remove has_traits variable from dependency logic * deindent helptext * de-indent helptext * de-indent helptext and fix grammar * Feature/hierarchy fields (#421) * add fields for hierarchy and hierarchy_details * add translations for hierarchy and details * Use msgcat to merge pofile with develop * Remove duplicated 'student_context' check And minor whitespace changes * Add extra text migrations for whitespace changes So they don't keep nagging us in the future. No actual changes to see here. * Merge rest of branch Co-authored-by: Michael Villeneuve <[email protected]> * fix fuzzy translation * add student_justification, student_context, and hierarchy to pdf * add translations * add filtertag * add fields to pdf * add newline * Fix/date start required (#426) * Add contingency for empty date_start in check_start_date() * Mark date_start as a soft required field in ProposalForm * merge migrations * add student detail fields to proposal diff * add hierarchy fields to study diff * EMA no longer is auto-long-route * update regulation url * update comment * remove n of sessions as reason * settings are no longer auto-long-route * setting auto-long-route logic removed * remove registrationkinds logic for auto-longroute * change name UiL OTS to ILS * create migration for namechange * add translations namechange * add yesno filter to hierarchy field in pdf * added missing td for diff proposal * add defaults for student_context * add filter to pdf for special traits * make helptext special_details translatable * add SpecialTrait model to admin * Revert "add filter to pdf for special traits" This reverts commit a10f7bf. * update traits description to mention medical details * include filter for special details in pdf * add special details to diff study * update docstring to reflect changes * update depends_on_value logic to work with secondary dependencies * update translations * migration medical details description * update logic for check_field_required * feature: speedy dependency checks * feature: intelligent unchecking of boxes * feature: intelligent unchecking of boxes * fix code formatting in template * refactor filter code same cleaning on another filter * fix: abort post request when a new request is started * formatting: fancy string to appease linter * specify the field for the postrequest * Fix some fuzzy or missing translations * fix: use map to track multiple requests Otherwise we might lose info * remove comment * add hierarchy and special details to auto_review * update auto_review tests * update longroute email * update shortroute email * fix: superfluous signoffs * update plaintext mails long and shortroute * add revision body for txt emails * additional update longroute txt * update html with revision versions * update translations * update menu header * remove menu item LK * update template to reflect unified regulations * Add translations for links in help menu * Removed more references to different chambers and added translations * use unique process identifier to fix bug * add edit dmp link for secretary * add translation for edit dmp view * add supervisor and creator to sidebar * Move html element up * Feature/remove passive consent (#439) Please note that because this branch was rebased, many commits have been duplicated below. Apologies for the dense log. * add fields for hierarchy and hierarchy_details * add translations for hierarchy and details * add fields for special details * fix inaccuracy with dependency check has_traits * change traits dependency to medical_traits * remove has_traits variable from dependency logic * add translations * add filtertag * add fields to pdf * add newline * update dependencies for urgent security fix * update dependencies for urgent security fix * fix: hide older versions of proposal in semi-public archive * feature: public archive * fix: added missing translations * fix: spelling * fix: spelling (again) * Fix/secure user uploads (#435) * UserMediaView new handles requests to /media/ for user uploads * Remove the now-unused static() user-uploads URL include * Serve user uploads with UserMediaView, securing them behind auth * REVERT previous raw push to master. Apologies * Fix/secure user uploads (#437) * UserMediaView new handles requests to /media/ for user uploads * Remove the now-unused static() user-uploads URL include * Check that requested user media paths are within MEDIA_ROOT * Remove extra media URL * Serve user uploads with new UserMediaView (#436) * Serve user uploads with new UserMediaView This prohibits non-logged-in users from accessing them * Prevent upwards directory traversal in UserMediaView * Remove extra user media url * Two content issues (#434) * Update homepage regulation links in Dutch and English * Missed some links * Removed old "updates" section that was very dated * Fixed links to the regulations in the pre-approval start page * Some missing spaces caused by PO trimming / html interactions * Proposal to application change * Remove unused translations * Hard-indent django templates at 4 spaces * Remove unused translations * Update master from acceptation (#432) * update dependencies for urgent security fix * fix: hide older versions of proposal in semi-public archive * feature: public archive * fix: added missing translations * fix: spelling * fix: spelling (again) * Fix/secure user uploads (#435) * UserMediaView new handles requests to /media/ for user uploads * Remove the now-unused static() user-uploads URL include * Fix/secure user uploads (#437) * UserMediaView new handles requests to /media/ for user uploads * Remove the now-unused static() user-uploads URL include * Check that requested user media paths are within MEDIA_ROOT * Remove extra media URL * Two content issues (#434) * Update homepage regulation links in Dutch and English * Missed some links * Removed old "updates" section that was very dated * Fixed links to the regulations in the pre-approval start page * Some missing spaces caused by PO trimming / html interactions * Proposal to application change * Remove unused translations * Hard-indent django templates at 4 spaces * Remove unused translations Co-authored-by: Michael Villeneuve <[email protected]> * Remove passive IC fields from form * Remove passive consent references from template and JS * Remove passive IC docs from ProposalSubmit validation * Remove passive IC from StudyForm * feat: new support email * Rewrite school consent form logic * Reduce template whitespace down to 2 spaces Apologies in advance to Ty *This commit changes no code and can be ignored* * Add indentation to hide_school_forms() * School forms are now wholly optional * Remove passive IC from pdf templates * Remove passive consent fields from model, add migration * Removed auto_review test for passive consent * Simplify Study.has_missing_forms() * merge migrations * EMA no longer is auto-long-route * update regulation url * update comment * remove n of sessions as reason * settings are no longer auto-long-route * setting auto-long-route logic removed * remove registrationkinds logic for auto-longroute * chore: fuzzy be gone! * add filter to pdf for special traits * make helptext special_details translatable * add SpecialTrait model to admin * Revert "add filter to pdf for special traits" This reverts commit a10f7bf. * update traits description to mention medical details * include filter for special details in pdf * add special details to diff study * update docstring to reflect changes * update depends_on_value logic to work with secondary dependencies * update translations * Fix/djang admin reviews list (#444) Fix bug in django-admin review editing * migration medical details description * pip-compile -U --resolver=backtracking * update logic for check_field_required * Merge some fuzzy translations * Fix broken link that regressed during merge * Update master from acceptation (#445) * Updated dependencies * New public archive * New user upload access view (This had already been pushed to master separately) * Two content issues (#434) * Update homepage regulation links in Dutch and English * Removed old "updates" section that was very dated * Fixed links to the regulations in the pre-approval start page * Some missing spaces caused by PO trimming / html interactions * Proposal to application change * Remove unused translations * Feature: new support email * Fix/django admin reviews list (#444) Co-authored-by: Ty Mees <[email protected]> * Fixed template referencing Documents object instead of Study object * Small dependency bump for security (#448) * feature: speedy dependency checks * feature: intelligent unchecking of boxes * feature: intelligent unchecking of boxes * refactor filter code same cleaning on another filter * Revert "Remove passive IC from pdf templates" This reverts commit 0bf437d. * Revert "Remove passive consent fields from model, add migration" This reverts commit d92c226. * fix: abort post request when a new request is started * formatting: fancy string to appease linter * specify the field for the postrequest * Fix template visibility of Passive IC fields and school forms * Visibility logic for additional forms to provide all 5 document fields. * Trim the block translations in proposal_pdf.html * Move Passive IC down to the bottom of the Study model with comment * Update verbose name of the external consent file fields * Add inconsequential migration for string changes * Add an infobox when any study requires the external forms * Add translations * Overlooked translation and word * Remove old translations * Fix some fuzzy or missing translations * fix: use map to track multiple requests Otherwise we might lose info * remove comment * add hierarchy and special details to auto_review * update auto_review tests * update longroute email * update shortroute email * fix: superfluous signoffs * update plaintext mails long and shortroute * add revision body for txt emails * additional update longroute txt * update html with revision versions * update translations * update menu header * remove menu item LK * update template to reflect unified regulations * Add translations for links in help menu * Removed more references to different chambers and added translations * use unique process identifier to fix bug * Added back is_school to some setting fixtures * Remove passive IC fields from form * Remove passive consent references from template and JS * Remove passive IC docs from ProposalSubmit validation * Remove passive IC from StudyForm * Rewrite school consent form logic * Reduce template whitespace down to 2 spaces Apologies in advance to Ty *This commit changes no code and can be ignored* * Add indentation to hide_school_forms() * School forms are now wholly optional * Remove passive IC from pdf templates * Remove passive consent fields from model, add migration * Removed auto_review test for passive consent * Simplify Study.has_missing_forms() * Fixed template referencing Documents object instead of Study object * Revert "Remove passive IC from pdf templates" This reverts commit 0bf437d. * Revert "Remove passive consent fields from model, add migration" This reverts commit d92c226. * Fix template visibility of Passive IC fields and school forms * Visibility logic for additional forms to provide all 5 document fields. * Trim the block translations in proposal_pdf.html * Move Passive IC down to the bottom of the Study model with comment * Update verbose name of the external consent file fields * Add inconsequential migration for string changes * Add an infobox when any study requires the external forms * Add translations * Added back is_school to some setting fixtures * Fix conflicting migrations * Fix study_form JS from rebase * Fix tests from rebasing onto new develop * Merge translations and fix empty POstring in proposal_diff.html --------- Co-authored-by: Meesch <[email protected]> Co-authored-by: Mees, T.D. (Ty) <[email protected]> Co-authored-by: Meesch <[email protected]> * update applicant name list in template * move dmp link to pdf container from document container * create update_attachment template for proposal * update translations * update translations (again) * update success url for update DM * update get_applicant_names_emails * hotfix: po translation duplicate line * chore: update locale file Needed because it was impossible to get a consistent merge * Nullify the minor revision attributes on copy (#482) * First commit to save changes * Fixed the translation for this feature * Edited the Poppler package to the correct one * Increase nr. of docs that can be added. Changed the value of 1 variable. * updated max num of words for ethical considerations field * created a validator for the comment fields and added COMMENTS_MAX_WORDS variable with value of 1000 * updated the string per field to showcase the correct num of words & translations * added the running wordcounter for the comments field * first attempt to refactor the word counter in its own function, but have not succeeded in displaying the formatted string 'aantal woorden' correctly. * fixed the bug by declaring translateable string outside of function call * Attempted to use the word_counter.js file for the proposal_form template, but it causes a bug, where the word counter only appears once, underneath the first text field * Implemented a working version of the seperate word counter static .js file. Works as it should now! * removed the jQuery shorthand inside of the static .js file and instead placed it in the template. * changed the other applicants if statement in the pre-approved and pre-assesment pdf templates to the one in the normal pdf template * change the cover image from the winter one, to the normal one. * changed UIL OTS to ILS in fixture institutions.json, and ran loaddata * changed on occurence of UIL OTS in the django.po file * Took a new cover image from the beeldbank and updated the credt * I don't think I included the changes ... But I did now, woops ;p * Added docs on merging translations --------- Co-authored-by: Meesch <[email protected]> Co-authored-by: Meesch <[email protected]> Co-authored-by: Michael Villeneuve <[email protected]> Co-authored-by: Edo Storm <[email protected]> Co-authored-by: EdoStorm96 <[email protected]>
* Update deps again, because why not * Add more failsafes to get_study_documents (#460) * Update dependencies * Fix error when study is None * Another req bump --------- Co-authored-by: Meesch <[email protected]> Co-authored-by: Ty Mees <[email protected]>
# Conflicts: # locale/en/LC_MESSAGES/django.mo # locale/en/LC_MESSAGES/django.po # main/templates/registration/login.html # requirements.txt
I think I merged the file properly, but just to be sure I'll update them again
Initial saml login support
miggol
requested changes
Sep 4, 2023
There was a problem hiding this comment.
It's a large PR so I haven't reviewed everything in detail, just ran through most of the changed pages locally. No problems found except for a missing migration.
Develop already has a proposals/0046 migration which is more elaborate than what is necessary here,. The changes are only to verbose_name fields so the effects are not breaking.
I suggest the following:
makemigrationsin this branch before merging- In develop, remove migration
proposals/0046and pull the smaller 0046 migration from acceptation - In develop, run
makemigrationsagain to add the additional changes back again as a new 0047 migration
But please reply to this review with alternate suggestions if there's a better way.
|
I'd like to propose an alternative: Do nothing :) Like you said, the changes that prompt Django's need to create a migration are not actually breaking. (The missing migration won't actually do anything!) The other way around: making the migration like you said is not 100% safe. Well, it is for production, but it will break the test server and, more importantly, anyone working from (a branch derived of) develop currently. While obviously those DB's are not holy, I don't think a missing no-op migration really warrants everyone to delete and recreate their DB. Yes, in a vacuum it's not ideal to have a missing migration, but honestly we've been running prod without this migration for months now. Why invest time in something that does not actually fix anything other than a 'correctness' issue (and that's coming from me), but will break dev environments? (In addition, I was planning on doing a merge/test/release cycle from dev after this PR was merged anyway, so the missing migration will arrive from dev in prod in, at most, weeks) |
Haha. I can live with doing nothing for the sake of the development servers, but these missing migrations issues have snowballed a bit in the past. Anyone who starts a feature branch off of acc has to think to include the migration manually or we'll be facing an annoying merge with migration re-ordering down the line. Even though the migrations do nothing, Django thinks they are critical. So I do see it as more than just a correctness issue. But the suggestion to do nothing is a valid one, so I'll change my review to approve this. |
miggol
approved these changes
Sep 4, 2023
|
Thanks, I'll merge this in a sec.
In general, I fully agree! And normally I'd be the one hammering down on this fact. Just in this specific case, it didn't make sense to me. However, I'd like to propose adding a new GH workflow that checks if a PR has missing migrations. This would hopefully allow us to catch missing migrations on fix/feature PR's before they even hit one of the main branches. It's a fairly easy workflow to write, just running |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No, this does not mean that SAML will immediately be enabled on the prod server.
It only means I can set it up and couple it to the IdP. The actual switching over to SAML will be done through a config change which will then replace the LDAP login button with the SAML login button.