-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate non csf certificates #14027
Conversation
…order to prevent memory leaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments
- Eyes test for this would be great, seems like the best way to see if we broke anything
- A sanity check controller test would be great as well
- Any reason not to fold CSF controller into this piece?
- Do we need to have access control restrictions on who can print a certificate? Right now I don't think we do but it's
@@ -11,5 +11,7 @@ def generate_certificate | |||
) | |||
|
|||
send_data image.to_blob, type: 'image/png', disposition: 'inline' | |||
ensure | |||
image && image.destroy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
image.try(:destroy!) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
workshop = @enrollment.workshop | ||
|
||
facilitator_names = workshop.facilitators.map {|f| f.name.strip} | ||
facilitator_fields = facilitator_names.each_with_index.map do |name, i| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any ordering of facilitator names we should do? Ask the PLC team this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Sure!
- Sure!
- Only that what I really want to do is entirely fold
create_certificate_image2
into this new method, but that's used in enough other places that that work really justifies its own PR - I don't know; do we? Are things like facilitator names considered private?
@@ -11,5 +11,7 @@ def generate_certificate | |||
) | |||
|
|||
send_data image.to_blob, type: 'image/png', disposition: 'inline' | |||
ensure | |||
image && image.destroy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
PTAL. Note that I cherry-picked a commit from this pending PR to help with the eyes test |
Reverting because the new eyes test is failing with
|
Note that the design of the core image - the way the title wraps around the name field - means that particularly long names will overlap the title:
However, also note that a quick look at the data confirms that we rarely need to deal with names over 20 characters
And names in that range fit just fine:
Note also that the facilitator names are implemented by stacking them, meaning that with a particularly large number of facilitators we could run into similar problems, but it would take quite a few for us to get to that point, and three facilitators (the maximum expected number) fit quite comfortably: