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

hoc: use DCDO value hoc_year in many places #15924

Merged
merged 4 commits into from Jun 29, 2017
Merged

Conversation

breville
Copy link
Member

@breville breville commented Jun 19, 2017

Rather than having to manually update a whole bunch of places with the year of the upcoming Hour of Code, instead use DCDO value hoc_year to specify that year.

This incorporates a bunch of suggestions given in #15735 to this end.

Note that it doesn't address the changes to www.hourofcode.com/events. From perusing those files, it seems there is an opportunity for a more significant code refactoring but I want to consider it separate to this change. Therefore this suggestion is not yet addressed.

@breville
Copy link
Member Author

@tanyaparker I've not yet embarked on testing these changes, and will probably need some help from you to cover everything. But I wanted to give you and @ashercodeorg an early look at the approach.

FORM_INFOS << {kind: "'HocSignup#{year}'", dest_field: "hoc_organizer_years", dest_value: "'#{year}'"}
end
FORM_INFOS << {kind: "'Petition'", dest_field: "roles", dest_value: "'Petition Signer'"}
FORM_INFOS.freeze
Copy link
Member Author

Choose a reason for hiding this comment

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

I maintained order here, though not sure whether that was necessary.

Copy link
Contributor

@tanyaparker tanyaparker left a comment

Choose a reason for hiding this comment

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

I haven't tested it or checked if you got all the places, but this set of changes lgtm. I'm prioritizing trying to get out back to school specs first, so might not have a chance to take a closer look at this until late this week.

Copy link
Contributor

@ashercodeorg ashercodeorg left a comment

Choose a reason for hiding this comment

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

Is a DCDO setting the most appropriate mechanism to specify the current HOC year? Not sure if there is better, I'm just wondering.

Otherwise, LGTM! Thanks for doing this!

@ashercodeorg
Copy link
Contributor

Looks like we missed a file, namely pegasus/routes/v2_form_routes.

@@ -76,11 +76,13 @@ def main
day += 1
end

hoc_year = DCDO.get("hoc_year", 2017)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should necessitate a require 'dynamic_config/dcdo'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Also found another similar case. @tanyaparker and I are scheduled to test all of this early next week to make sure it's working as expected, before merging.

@tanyaparker
Copy link
Contributor

@breville

I didn't test these and need your help testing locally.

  1. Run bin/cron/analyze_hoc_activity
    1a. I got errors for this locally even on staging branch.
  2. Run bin/cron/update_hoc_map
    2a. Make sure you see id # 6053412 in fusion table
    https://fusiontables.google.com/DataSource?docid=164RLDWEyPij3-Z9O0791ta_a8RfuCdGUkWRFcnBD#rows:id=1
  3. Test that you see custom hoc scale messages.
    3a. Sign up on hoc.com with your email
    3b. Turn on scale mode
    3c. When logged out, try to create an account
    3d. You should see a custom message about the Hour of Code and not needing to create an account to participate.

I didn't test these but can test on staging.

  1. Get correct "thank you email" from signing up a hoc event
  2. See hoc events on /events page (could see production data on local)

I tested this.

  1. Can sign up for hoc event

@ashercodeorg
Copy link
Contributor

ashercodeorg commented Jun 27, 2017

Re: (1a), I'm working on a fix for this.

@tanyaparker, my assumption is that you are seeing the same only_full_group_by errors that I'm seeing?

@tanyaparker
Copy link
Contributor

@ashercodeorg I actually got a nil error on line 137.

existing_lines_of_code = Properties.get(:metrics)['lines_of_code']

@ashercodeorg
Copy link
Contributor

Ah, that is much, much simpler to fix. From pegasus-console run:

  1. require 'cdo/properties'
  2. Properties.set :metrics, {'lines_of_code' => 123}
    Though it might make sense to guard against the key missing (both :metrics and 'lines_of_code'), I'm not certain that change makes sense in all environments. This'll fix it for you locally.

@tanyaparker
Copy link
Contributor

That fixed it thank you!

Conflicts:
	pegasus/sites.v3/hourofcode.com/public/events/splat.haml
@breville
Copy link
Member Author

Regarding those three scenarios to test:

  1. I'm told that was fixed.
  2. Testing that separate with the PR to update the API.
  3. Even in scale mode I was able to sign up an event on hourofcode.com and a user on code.org. There is a custom message shown when trying to sign up with an email already used for an event, but that seems independent of scale mode.

Merging now.

@breville breville merged commit b255809 into staging Jun 29, 2017
@breville breville deleted the hoc-dynamic-year branch June 29, 2017 04:17
@tanyaparker
Copy link
Contributor

Testing in staging... Unfortunately the Spanish/Portuguese emails don't work, you always get English. But this just keeps the existing behavior (it never worked). So I'll track to fix when we do more hoc work after back to school.

@ashercodeorg
Copy link
Contributor

Do we have an understanding why the i18n emails aren't sent? If we do, I imagine it wouldn't be too hard to fix.

@tanyaparker
Copy link
Contributor

My guess is that we don't know @country at the time that we're choosing which email template to send. I know we must know it when someone signs up for hoc because we track sign ups per country page. But not sure if it passes all the way to the email template.

@ashercodeorg
Copy link
Contributor

Looks like you are right, I'm not seeing the @country variable getting set anywhere. That said, it should be easy enough to set.

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

Successfully merging this pull request may close these issues.

None yet

4 participants