Only allow teachers who attend workshops to print workshop certificates #32678

bencodeorg · 2020-01-15T00:15:35Z

PLC-687: Prevents teachers who did not attend workshops from printing certificates of completion.

Testing story

Added a test for the new behavior. I was trying to test by actually generating a click event, but I ran into this bug (I think). In order to get feedback I added a less great test, but happy to adjust if there are ideas about how to work around this issue.

Also added/modified testing related to new server side logic to prevent rendering workshop certificates for teaches who haven't attended a workshop.

Reviewer Checklist:

Tests provide adequate coverage
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

dashboard/app/controllers/api/v1/pd/workshops_controller.rb

bencodeorg · 2020-01-15T00:19:46Z

apps/test/unit/code-studio/pd/professional_learning_landing/EnrolledWorkshopsTest.jsx

    const enrolledWorkshopsTable = shallow(
      <EnrolledWorkshopsTable workshops={workshops} />
    );

    // Click the "Print Certificate" button
    enrolledWorkshopsTable
-      .find('tr')
-      .last()
+      .find('tbody tr')


Changed this find call because there is a header row that had me banging my head against the wall for a bit.

islemaster · 2020-01-15T00:26:29Z

Going to get to a review of what you've got here, but first: What you've got here is a good client-side improvement for the user experience when viewing a workshop you didn't attend, but it doesn't actually prevent someone from going to studio.code.org/pd/generate_workshop_certificate/<code> and printing their unearned certificate. For that, we should make a server-side change: Let's update WorkshopCertificateController to check for attendance before generating the certificate at all.

islemaster · 2020-01-15T00:29:42Z

apps/src/code-studio/pd/professional_learning_landing/EnrolledWorkshops.jsx

@@ -78,6 +78,7 @@ class EnrolledWorkshopsTable extends React.Component {
          <Button
            onClick={() => this.openCertificate(workshop)}
            style={styles.button}
+            disabled={!workshop.attended}


👍 The way you've serialized out the relevant data makes this line, where we eventually use it to make a decision, so nice. Good choice.

islemaster · 2020-01-15T00:33:12Z

apps/test/unit/code-studio/pd/professional_learning_landing/EnrolledWorkshopsTest.jsx

-      state: 'Not Started'
+      state: 'Not Started',
+      user_id: 123,
+      attended: false


Not required right now, but seeing this large fixture again, and how many places you had to edit it, reminded me that we have a better way to generate test data now: Rosie.js, the JavaScript equivalent to FactoryBot. Here's an example of cleaning up a large test fixture with Rosie.

islemaster · 2020-01-15T00:37:45Z

apps/test/unit/code-studio/pd/professional_learning_landing/EnrolledWorkshopsTest.jsx

+      .find('Button')
+      .first();
+
+    expect(printCertificateButton.prop('disabled')).to.be.true;


The fact that disabled wasn't respected on this simulated button click may be by design - in particular, we're shallow-rendering here, and using a bootstrap Button component, so in theory we're not even rendering the DOM element that would normally enforce disabled as a behavior.

I think your approach here, checking the prop itself, is fine. Just make sure to update the comment on line 146 since we're not actually clicking the button anymore.

islemaster

Great first pass! I have a few comments to address before we move forward here.

bencodeorg · 2020-01-15T17:32:58Z

Going to get to a review of what you've got here, but first: What you've got here is a good client-side improvement for the user experience when viewing a workshop you didn't attend, but it doesn't actually prevent someone from going to studio.code.org/pd/generate_workshop_certificate/<code> and printing their unearned certificate. For that, we should make a server-side change: Let's update WorkshopCertificateController to check for attendance before generating the certificate at all.

@islemaster Can you think of an example you could share of how we gracefully handle this in other places?

islemaster · 2020-01-15T17:52:49Z

One good example of this pattern of catching business logic edge cases in the controller is the route for cancelling a workshop enrollment. It takes an enrollment code as part of the URL.

GET /pd/workshop_enrollment/<code>/cancel

This route handles two special edge cases. One, perhaps we can't find an enrollment for the code you provided at all. In that case, we render this view saying the registration was not found.

    @enrollment = Pd::Enrollment.find_by_code params[:code]
    if @enrollment.nil?
      render :not_found

Two, maybe we find the enrollment, but you already attended the workshop, so it's too late to cancel! In that case, we render a view that says "You have already attended this workshop."

    elsif @enrollment.attendances.any?
      render :attended

Finally, if neither of the above happens, we fall through and render the cancellation form.

I think you could take a similar approach, or since we wouldn't expect someone to open this URL unless they were actually trying to bypass our UI, we could simply return a 404 if they didn't attend the workshop. After all, they didn't earn a certificate yet, so not found seems like a reasonable response.

return head :not_found unless enrollment.attendances.any?

bencodeorg · 2020-01-15T23:35:02Z

Hold on review, need to fix some tests.

… attend

islemaster · 2020-01-16T16:54:07Z

dashboard/test/controllers/pd/workshop_certificate_controller_test.rb

+
+    get :generate_certificate, params: {enrollment_code: @enrollment.code}
+    assert_response :missing
+  end


🎉 Awesome.

islemaster · 2020-01-16T17:00:35Z

dashboard/app/controllers/pd/workshop_certificate_controller.rb

@@ -6,6 +6,8 @@ class Pd::WorkshopCertificateController < ApplicationController
    find_by: :code, id_param: :enrollment_code

  def generate_certificate
+    return render_404 unless @enrollment&.attendances&.any?


I think the first safe-navigation operator isn't needed here because the load_resource line above that populates @enrollment should cause a 404 if it's not found before we reach this code; and the second safe-navigation operator shouldn't be necessary because if you have an Enrollment with no attendances, its attendances association will give an empty array, not nil. But use tests to check these cases before taking my word for it!

Suggested change

return render_404 unless @enrollment&.attendances&.any?

return render_404 unless @enrollment.attendances.any?

Makes sense -- we do have a test that an error is raised if a nonsense enrollment code is provided, so I think your first assumption is sound. I tested manually in console that an enrollment with no attendances returns false from enrollment.attendances.any?, not an error. Also tested the generate_workshop_certificate page with this code change and an enrollment with no attendance got the 404 I'd expect.

islemaster · 2020-01-16T17:01:18Z

dashboard/app/serializers/api/v1/pd/workshop_serializer.rb

@@ -63,6 +63,10 @@ def enrollment_code
    @scope.try(:[], :enrollment_code)
  end

+  def attended
+    object.enrollments.find_by(code: @scope.try(:[], :enrollment_code))&.attendances&.any?


Whereas here we need the two safe-navigation operators because find_by might have returned nil.

islemaster

Great work!

Only allow teachers who attend workshops to print workshop certificates

a12555e

bencodeorg requested review from islemaster and molly-moen January 15, 2020 00:15

bencodeorg commented Jan 15, 2020

View reviewed changes

dashboard/app/controllers/api/v1/pd/workshops_controller.rb Outdated Show resolved Hide resolved

bencodeorg commented Jan 15, 2020

View reviewed changes

islemaster reviewed Jan 15, 2020

View reviewed changes

islemaster suggested changes Jan 15, 2020

View reviewed changes

bencodeorg added 2 commits January 15, 2020 12:42

Move attendance status into workshop serializer

ca956dc

Prevent rendering workshop certificates on server

a4b69fd

bencodeorg closed this Jan 15, 2020

bencodeorg reopened this Jan 15, 2020

bencodeorg requested a review from islemaster January 15, 2020 21:19

Update tests for printing workshop certificates only for teachers who…

c1b938f

… attend

islemaster reviewed Jan 16, 2020

View reviewed changes

islemaster approved these changes Jan 16, 2020

View reviewed changes

Remove unnecessary safe navigation operator

919f893

bencodeorg merged commit 6d65591 into staging Jan 16, 2020

bencodeorg deleted the pd-certificates-for-workshop-attendees branch January 16, 2020 21:35

This was referenced Jan 16, 2020

Revert "Only allow teachers who attend workshops to print workshop certificates" #32738

Merged

Revert "Revert "Only allow teachers who attend workshops to print workshop certificates"" #32739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow teachers who attend workshops to print workshop certificates #32678

Only allow teachers who attend workshops to print workshop certificates #32678

bencodeorg commented Jan 15, 2020 •

edited

bencodeorg Jan 15, 2020

islemaster commented Jan 15, 2020

islemaster Jan 15, 2020

islemaster Jan 15, 2020

islemaster Jan 15, 2020

islemaster left a comment

bencodeorg commented Jan 15, 2020

islemaster commented Jan 15, 2020 •

edited

bencodeorg commented Jan 15, 2020

islemaster Jan 16, 2020

islemaster Jan 16, 2020

bencodeorg Jan 16, 2020

islemaster Jan 16, 2020

islemaster left a comment

	return render_404 unless @enrollment&.attendances&.any?
	return render_404 unless @enrollment.attendances.any?

Only allow teachers who attend workshops to print workshop certificates #32678

Only allow teachers who attend workshops to print workshop certificates #32678

Conversation

bencodeorg commented Jan 15, 2020 • edited

Testing story

Reviewer Checklist:

Choose a reason for hiding this comment

islemaster commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

bencodeorg commented Jan 15, 2020

islemaster commented Jan 15, 2020 • edited

bencodeorg commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

bencodeorg commented Jan 15, 2020 •

edited

islemaster commented Jan 15, 2020 •

edited