Skip to content
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

write CSF course name onto blank certificate #46829

Merged
merged 28 commits into from
Jun 21, 2022
Merged

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Jun 10, 2022

Background

Finishes PLAT-1735. Here is the current state of the world:

Screen Shot 2022-06-13 at 9 07 50 AM

Most importantly, when completing a course:

  • completing a HOC course will always take you to the studio congrats page
  • completing a CSF course will take you to the studio congrats page only if you have the experiment enabled

For more diagrams and next steps, see congrats page work plan.

Description

The goal of this PR is that when you complete a CSF course with the experiment enabled, you should see the course name printed onto a blank certificate (see 1st screenshot). In order to accomplish that, this PR does the following things:

  1. add default_random_donor param to create_course_certificate_image, and use it to limit random donor selection so that this method never provides a random donor when called from dashboard (e2b1624)
  2. relax requirements on /certificate_images/ routes so that student name and donor name can be omitted (7ce1869)
  3. use custom certificate with course name written onto it (rather than default HOC certificate) for "blank" CSF certificates (fb4d4de)
  4. remove client logic for displaying blank HOC certificate, and use existing server logic instead (e6091ca, 26c633f)

Steps 1-3 are necessary to get blank csf certificates to appear properly on /certificates/ and /print_certificates/ pages (see 1st screenshot/video for a refresher). step 4 is also necessary to get blank csf certificates to appear properly on /congrats/.

Also worth noting that step 4 is a "visible" change even for users without the experiment, in the sense that it changes the url used to display the certificate image for HOC tutorials. However, the image itself should be identical (see last screenshot).

Screenshots

new behavior for finishing CSF congrats page, with experiment:

Screen.Recording.2022-06-13.at.9.37.24.AM.mov

after going to code.org/congrats/course1?enableExperiments=studioCertificate (before / after):

Screen Shot 2022-06-13 at 9 17 11 AM

after completing the oceans tutorial, with or without experiment (before / after):

Screen Shot 2022-06-13 at 9 22 13 AM

no image rendered with a bogus course name (just trying to keep us out of trouble here):

Screen Shot 2022-06-13 at 5 25 38 PM

Testing story

  • updated and extended unit test coverage on rails controllers and react components
  • updated existing UI tests and added new UI / eyes tests

Follow-up work

@davidsbailey davidsbailey changed the title Customize csf blank write CSF course name onto blank certificate Jun 13, 2022
@davidsbailey davidsbailey requested review from a team June 15, 2022 03:16
@davidsbailey davidsbailey marked this pull request as ready for review June 15, 2022 03:16
@megcrenshaw
Copy link
Contributor

megcrenshaw commented Jun 16, 2022

Your screencasts are so helpful. I'm having trouble verifying this works locally though:

I pulled your branch, ran yarn build and ran dashboard-server ... is there something else I need to do before trying it?

Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me (but I didn't try to pull the branch and run it like Meg's doing). Thanks for continuing to push on this!

@davidsbailey
Copy link
Member Author

davidsbailey commented Jun 17, 2022

Apologies, Meg. For some reason, you have to be running the dashboard server on Linux for writing onto certificates to work. I tried to set up an adhoc, but it kept failing due to known issues which infra is working on, so I took the screenshots against a server running on a developer EC2 instance.

I think that explains the first 2 anomalies, but I'll have to look into the 3rd next week.

@davidsbailey
Copy link
Member Author

@megcrenshaw I think this looks like you may not have had the experiment enabled on dashboard. unfortunately, you have to enable it on both dashboard AND pegasus. you can see this happening in the UI tests:

Scenario: Course A 2017 uncustomized dashboard certificate pages
Given I am on "http://studio.code.org/congrats?enableExperiments=studioCertificate"
And I wait until element "#uitest-certificate" is visible
When I am on "http://code.org/congrats/coursea-2017?enableExperiments=studioCertificate"

@megcrenshaw
Copy link
Contributor

@megcrenshaw I think this looks like you may not have had the experiment enabled on dashboard. unfortunately, you have to enable it on both dashboard AND pegasus. you can see this happening in the UI tests:

Ah, thanks! That was the problem. Name doesn't come through locally, but it's not the Hour of Code certificate:

image

mee: require('@cdo/static/MC_Hour_Of_Code_Certificate_mee.png'),
mee_empathy: require('@cdo/static/MC_Hour_Of_Code_Certificate_mee_empathy.png'),
mee_timecraft: require('@cdo/static/MC_Hour_Of_Code_Certificate_mee_timecraft.png')
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hurray deleted files!

@@ -33,13 +31,20 @@ class CertificateImagesControllerTest < ActionController::TestCase
assert_includes response.body, 'invalid donor name'
end

test 'can show certificate image given bogus course name' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you added a test to document undesired behavior and then revised the test once the functionality was as desired

Copy link
Contributor

@megcrenshaw megcrenshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great! I verified what I could locally. Nice work, and thanks for the thorough documentation

@davidsbailey
Copy link
Member Author

Thank you both for the thorough review! I tried one last time to start an adhoc, but no luck 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants