Make buttons work #13473
Make buttons work #13473
Conversation
| }, | ||
|
|
||
| onPrintCertificateClick() { | ||
| const enrollmentCode = this.props.lastWorkshopSurveyUrl.substring(this.props.lastWorkshopSurveyUrl.lastIndexOf('/')); |
There was a problem hiding this comment.
It seems a bit odd to parse the enrollment code here, when this url was constructed from the enrollment code in the first place.
What about passing both urls, or even better passing just the enrollment code and constructing both urls in the client.
There was a problem hiding this comment.
The latter sounds good to me
| enrollment.try(:name) || '', | ||
| y: 444, | ||
| height: 100 | ||
| ) |
There was a problem hiding this comment.
Do we want to just display a blank certificate on a bad enrollment code? I think 404 (not found) would make more sense.
Incidentally you get that for free with load_resource (above)
|
|
||
| def generate_certificate | ||
| enrollment = params[:enrollment_code] ? Pd::Enrollment.find_by(code: params[:enrollment_code]) : nil | ||
|
|
There was a problem hiding this comment.
This could also be loaded into @enrollment via CanCan's load_resource before action hook:
load_resource :enrollment, class: 'Pd::Enrollment', find_by: :code, id_param: :enrollment_codeThere was a problem hiding this comment.
Ah - didn't know you could load via code. I'll do it that way
| const csFundamentalsSection = landingPage.find('CsFundamentalsSection'); | ||
| expect(csFundamentalsSection).to.have.length(1); | ||
| expect(csFundamentalsSection.prop('lastWorkshopSurveyUrl')).to.equal(null); | ||
| expect(csFundamentalsSection.prop('printCsfCertificateUrl')).to.equal('otherUrl'); |
There was a problem hiding this comment.
nit: 'certificateUrl' instead of 'otherUrl'
| def generate_certificate | ||
| image = create_certificate_image2( | ||
| dashboard_dir('app', 'assets', 'images', 'pd_workshop_certificate_csf.png'), | ||
| @enrollment.try(:name) || '', |
There was a problem hiding this comment.
Pd::Enrollment.name is deprecated. Use full_name instead.
As an aside, we need to remove name and other no longer used fields at some point.
| class Pd::CsfCertificateController < ApplicationController | ||
| before_action :authenticate_user! | ||
| load_resource :enrollment, class: 'Pd::Enrollment', find_by: :code, id_param: :enrollment_code | ||
|
|
There was a problem hiding this comment.
please add some basic tests for this controller:
- authenticated users can access
- non-users cannot
- valid enrollment code works
- invalid enrollment code gets not found
| @@ -15,13 +15,20 @@ def index | |||
|
|
|||
| summarized_plc_enrollments = Plc::UserCourseEnrollment.where(user: current_user).map(&:summarize) | |||
|
|
|||
| if courses_completed.include?(Pd::Workshop::COURSE_CSF) | |||
| enrollment = Pd::Enrollment.where(pd_workshop_id: ended_workshops.where(course: Pd::Workshop::COURSE_CSF).map(&:id), email: current_user.email).last | |||
There was a problem hiding this comment.
These are not ordered, so last won't necessarily be consistent.
Please add a test to verify this is working correctly, that it gets the latest enrollment for the given user in a CSF ended workshop, disregarding enrollments from other users and other workshops.
Make sure you also check user_id: current_user.id. In case a user changes email after the fact: where('email = ? OR user_id = ?', current_user.email, current_user.id)
This logic might better fit the enrollment model, with testing there. At least the user part.
There was a problem hiding this comment.
This logic might better fit the enrollment model, with testing there. At least the user part.
Like a scope?
There was a problem hiding this comment.
Yes, either a scope or static method.
| get :generate_certificate, params: {enrollment_code: @enrollment.code} | ||
| assert_response :redirect | ||
| end | ||
| end |
There was a problem hiding this comment.
I just realized, after seeing these tests, that any enrollment code will work on this url, not just those for ended workshops. Not necessary in this PR, but we might want to add some more validation.
No description provided.