Cosign improvements (batch 2) #4838
Merged
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
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
2 times, most recently
from
40e5f3f to
829a491
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4838 +/- ##
==========================================
- Coverage 96.61% 96.61% -0.01%
==========================================
Files 751 752 +1
Lines 25679 25737 +58
Branches 3395 3406 +11
==========================================
+ Hits 24809 24865 +56
- Misses 608 610 +2
Partials 262 262 ☔ View full report in Codecov by Sentry. |
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
2 times, most recently
from
36ced0e to
8d6a2ad
Compare
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
from
8d6a2ad to
04c4d31
Compare
|
Putting this one up for review - however keep in mind that I want to do a refactor follow-up PR:
|
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
from
04c4d31 to
774fad2
Compare
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
3 times, most recently
from
ef50566 to
3da92e2
Compare
if no form fields are going to be rendered in the summary, there should not be any heading emitted. Also relevant for the (duplicate) #4831
…ng to report The confirmation email is only supposed to emit the summary heading if there's actual data being summarized. This relies on the renderer having children or not, since the actual table is produced by the renderer/tree visitor walking over the submission (steps) and associated form components in the form definition. The check whether the render has children was faulty since it returned children that didn't have any grand-children itself, wrongly creating the impression that there's going to be content output. There was some old, commented out, prototype code that hinted at this not being the intent, that was replaced with a proper check. This also fixes #4831, which is a duplicate of a bullet in the main cosign improvements ticket.
Since we've committed to this route by doing this for the confirmation page content template, we have to be consistent and follow the same pattern for the confirmation email. There's an extra complexity here because the confirmation email templates can be overridden at the form level.
Added additional (translatable) template fields to the global configuration and form-specific confirmation email templates to render for submissions requiring cosigning. TODO: expose them in the admin, API and frontend.
Refactored the template selection for subject and content of the confirmation email to take into consideration if cosigning is required or not.
* Expose the fields in the serializers * Updated the frontend code to add the fields * Updated the end to end tests asserting the translation warnings
The content of the header needs to change depending on the cosign state, and we now provide more control in the parent templates so the most flexible solution is to just remove the header from the hardcoded template. While we're working on 3.0 which has breaking changes, this is the best coincidence we can hope for to make this breaking change.
…cosign status A submission requiring cosigning is still in a waiting state before it will be sent to a registration backend. The language used in the PDF now makes this clear, which should also avoid legal implications associated with the displayed 'completion' timestamp.
Simplified the language down to B1 level.
sergei-maertens
force-pushed
the
feature/4320-cosign-improvements-2
branch
from
3da92e2 to
d9c6e9d
Compare
vaszig
approved these changes
Nov 27, 2024
| @@ -1,3 +1,5 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Do we need that in python 3.12? Or are we doing this for earlier versions?
There was a problem hiding this comment.
this mostly makes the annotations/imports lazy so you can use the Name in places when doing conditional imports without having to wrap them in strings: "Name"
| @@ -0,0 +1,42 @@ | |||
| import functools | |||
There was a problem hiding this comment.
Are we going to keep this solution?I thought it was not the what we wanted.(asking since this is now a reusable thing..)
There was a problem hiding this comment.
it's not what we want, but it is what we need for the time being 😬
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.
Closes #4320 (partially)
Changes
Screenshots
E-mail to submitter before cosigning
E-mail to submitter after cosigning
PDF generated
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shCommit hygiene