add page to dashboard for batch printing of certificates - #48000
Conversation
7d19ce6 to
e27155e
Compare
e2bae3e to
299f9c7
Compare
davidsbailey
changed the title
davidsbailey
changed the title
| #certificate-batch { | ||
| p { | ||
| font-size: inherit; | ||
| line-height: inherit; |
There was a problem hiding this comment.
this is to override these styles:
code-dot-org/dashboard/app/assets/stylesheets/scaffolds.scss
Lines 10 to 12 in c1efb72
without this, SafeMarkdown renders p elements whose font size and line height can not be overridden by the parent component.
|
LGTM thanks Dave! |
| view_options(no_header: true, no_footer: true, white_background: true, full_width: true) | ||
|
|
||
| student_names = params[:studentNames]&.split("\n")&.map(&:strip)&.shift(30) | ||
| course_name = params[:courseName].presence || ScriptConstants::HOC_NAME |
| def batch | ||
| view_options(no_header: true, no_footer: true, white_background: true, full_width: true) | ||
|
|
||
| student_names = params[:studentNames]&.split("\n")&.map(&:strip)&.shift(30) |
There was a problem hiding this comment.
I see the instruction above that this will only print 30 names, but it looks like this will still "work" if there are more than 30 names, with no warning that some names didn't get printed. Is there a plan for a warning/error?
There was a problem hiding this comment.
I'm also curious where this limit comes from –– teachers can easily have 120 students
There was a problem hiding this comment.
good points. to answer questions literally:
-
there is this "warning" ahead of time:
-
there is not a plan to add a better warning here. the specced behavior is just to match parity with the existing pages.
I'm not sure where the earlier limit came from, but I think we do want some upper bound, since generating images can be expensive for our servers, and attacks are often measured by how much load you can put on a server per web request.
@tess323 do you have a thought here on whether a better warning experience is worth doing?
| export default function CertificateBatch({courseName, imageUrl}) { | ||
| return ( | ||
| <div style={styles.wrapper}> | ||
| <h1>Print a batch of certificates</h1> |
There was a problem hiding this comment.
Should this be translated?
4 participants
Starts PLAT-1533. Sets up the new batch certificate pages on dashboard. The next PR will make existing pages on pegasus redirect here and will make teacher dashboard link here. See requirement #5 and background image 4 in the spec. This implements the highlighted part of the diagram seen in the work plan:
screenshots
new behavior
basic functionality (saucelabs video):
print-certificates-uitest.mp4
instructions are hidden when printing:
when a student tries to access studio.code.org/certificates/batch:
before & after
configure certificates
print certificates
Testing story
new unit tests, ui tests and eyes tests, plus existing test coverage.
the one thing that doesn't have a good unit test is the CertificateBatch react component. This is difficult to test because the implementation uses a form post rather than an ajax request, and form posts cannot be stubbed out with sinon. however the component is very simple and its behavior is well covered by the UI test, so it doesn't quite seem worth upgrading to an ajax-based approach until/unless we plan to do a lot more here to upgrade the component.
Deployment strategy
Safe to deploy this PR without experiment flag since no traffic is pointing to it yet. the next PR will only link/redirect to this page conditionally, based on the experiment flag.
Follow-up work
note that the new page prints "hourofcode" (the script name) instead of "Hour of Code" (the script display name). This is covered by https://codedotorg.atlassian.net/browse/PLAT-1888 and will be fixed before the experiment is launched.