-
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
AFE: move submit logging to server #35723
Changes from 3 commits
62ba85f
a5b1c56
924ec5a
380723c
6bcd64e
830d83d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
require 'honeybadger/ruby' | ||
require 'cdo/firehose' | ||
|
||
# | ||
# Handles submissions to our AFE form at code.org/afe, and in turn submits on | ||
|
@@ -12,7 +13,7 @@ def submit | |
return head :forbidden unless current_user&.teacher? | ||
|
||
afe_params = submit_params | ||
Services::AFEEnrollment.submit( | ||
submission_body = Services::AFEEnrollment.submit( | ||
first_name: afe_params['firstName'], | ||
last_name: afe_params['lastName'], | ||
email: afe_params['email'], | ||
|
@@ -29,6 +30,20 @@ def submit | |
new_code_account: current_user.created_at > 5.minutes.ago | ||
) | ||
|
||
FirehoseClient.instance.put_record( | ||
{ | ||
study: 'amazon-future-engineer-eligibility', | ||
event: 'submit_to_afe', | ||
data_json: { | ||
accountEmail: current_user.email, | ||
accountSchoolId: current_user&.school_info&.school&.id, | ||
formEmail: afe_params['email'], | ||
formSchoolId: afe_params['schoolId'], | ||
formData: submission_body | ||
}.to_json | ||
} | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we're tracking this event if we successfully submit to AFE or if submitting to AFE is disabled for that environment. (But not if we skip submission or submission fails for some other reason.) That seems okay - is it the desired behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it was sort of for testing purposes, so I could see what would be sent to Firehose in development (since it sends Firehose requests to your logfile). Seems maybe useful in the future as well, to have the entire payload returned to the controller in development (instead of just immediately returning)? |
||
|
||
# If the teacher requested it, submit to CSTA as well | ||
if to_bool(afe_params['csta']) | ||
school = School.find_by(id: afe_params['schoolId']) | ||
|
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.
Removing the href may have changed the style of this link. I think, in this case, you could avoid that with
href="#"
or something - I don't feel bad about changing the hash since we're about to navigate.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.
Nice, good to know will update! Main difference I see is that when you hover it gives you the clicky finger with an href, and the text cursor without it.
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.
fwiw, you can use
cursor: pointer
to show the clicky finger again.