PL Launch 2023 - Show default homepage hero banner to int'l audiences by kelbyhawn · Pull Request #51356 · code-dot-org/code-dot-org

kelbyhawn · 2023-04-14T19:44:37Z

Show the default homepage hero banner to international audiences and the PL superhero banner to US only.

Asana task: https://app.asana.com/0/0/1204402039772899

Related PR:

PL Launch 2023 - Homepage hero banner #49588

Testing story

[from Meg] The homepage.rb file currently has no tests. To convince myself that the showInternationally flag was working as expected, I used the announcements test file for inspiration.

I had three attempted testing strategies and landed on the last one:

I was originally going to stub out the @@announcements_data variable using Homepage.class_variable_set(:@@announcements_data, {...}). But that approach meant that the vast majority of the test file was hard-to-parse JSONs, or hard-to-read updates to a single JSON.
Then, I was going to use a separate file as the announcements test file does. But we're needing to test banners with different showInternationally flags, and our current banner configuration returns the very first banner it finds:

code-dot-org/pegasus/src/homepage.rb

Lines 34 to 35 in 0cc37cc

# We have a banner. Add the ID to the hash that we return.

return banner.merge({id: banner_id_for_page})
I ended up creating a separate file for each of the showInternationally flag configurations –– true, false, and no flag. I don't love that there is quite a bit of repeated code for each JSON, but I do think it is more readable than the approach in (1).

Links

[from Meg] Asked Platform about best practices for querying location here https://codedotorg.slack.com/archives/CFTFD6BPV/p1681909723548929?thread_ts=1680634885.139079&cid=CFTFD6BPV

megcrenshaw

I'm confused about the product requirement –– do we want to show the page for English users or for users inside the US? It looks like this PR will show the banner for any English user, including English users in, say, India or Australia.

dmcavoy · 2023-04-17T12:51:46Z

Good question Meg. The goal is to show the PL banner just to United States users. Is it possible to do that instead of relying on language to try to capture that?

kelbyhawn · 2023-04-17T15:03:16Z

Good question Meg. The goal is to show the PL banner just to United States users. Is it possible to do that instead of relying on language to try to capture that?

I don't think we have the functionality for the homepage.json banners to show based off of geolocation (yet?) — I'll look into it.

kelbyhawn · 2023-04-17T15:25:11Z

For more context — this is what we did last year for the CSA launch hero banner that had similar audience requirements (Brendan helped me pair on this):

CSA Launch - website updates pt. 1 #46406 (file ref)

megcrenshaw · 2023-04-17T16:11:39Z

Looks like this is where we set the language config:

code-dot-org/pegasus/src/homepage.rb

Lines 31 to 32 in 062d9ef

    
           # If the banner has an array of languages, then the current language must be one of them. 
        
           next if banner["languages"] && !banner["languages"].include?(request.language)

I believe we could use a similar pattern for country, but it's not currently configured. Depending on how high priority this is, one of us could implement this functionality for, Kelby, you to use. (Kelby, I'm sure you could get this working too, but I'm guessing learning Ruby wasn't part of the job description 😅 .)

dmcavoy · 2023-04-17T16:13:27Z

@megcrenshaw thank you for looking into this. This is high priority as we are sending lots of traffic to pages that don't make any sense for them. Could you send some time today or tomorrow helping Kelby to get this working?

kelbyhawn · 2023-04-17T16:14:21Z

Looks like this is where we set the language config:

code-dot-org/pegasus/src/homepage.rb

Lines 31 to 32 in 062d9ef

# If the banner has an array of languages, then the current language must be one of them.

next if banner["languages"] && !banner["languages"].include?(request.language)

I believe we could use a similar pattern for country, but it's not currently configured. Depending on how high priority this is, one of us could implement this functionality for, Kelby, you to use. (Kelby, I'm sure you could get this working too, but I'm guessing learning Ruby wasn't part of the job description 😅 .)

I just tried that in this commit but it wasn't working 😅 maybe I missed something though?

megcrenshaw · 2023-04-17T16:22:46Z

@dmcavoy Yep, on it!
@kelbyhawn Ah, brilliant! I missed that. You're way ahead of me. The request object doesn't include the country, but we can use this pattern:

location = Geocoder.search(user.current_sign_in_ip).first
country = location&.country_code&.to_s&.downcase

Want to try the following?

# If the banner has an array of countries, then the current country must be one of them.
location = Geocoder.search(user.current_sign_in_ip).first
country = location&.country_code&.to_s&.downcase
next if banner["countries"] && !banner["countries"].include?(country)

Then try us (lowercase) in the JSON. Or you can do &.upcase in the ruby file (instead of &.downcase) with capitalized US in the JSON –– whatever feels more intuitive to you.

If that doesn't work, let me know and we can either pair on it or I can take a deeper look solo. Thanks, Kelby, for starting on this!

davidsbailey · 2023-04-18T16:33:58Z

+      # If the banner is shown internationally don't show in the US.
+      # If the banner is not shown internationally, show in the US.
+      location = Geocoder.search(request.ip)&.first
+      in_us = location&.country_code.to_s.casecmp?('us')
+      next unless banner["showInternationally"] ^ in_us


questions:

how often is request.ip set? is it as reliable as user.current_sign_in_ip?

does this codepath work (no errors) when request.ip is nil?

in the case where request.ip is nil, are we ok with showing only outside us?

I'm not sure about taking away the option to display banners globally. isn't this something we'll want to do for HOC?

Good questions.
1-2) We can't use current_sign_in_ip because the user may not be signed in and we're on pegasus. But I don't know how often request.ip is set.
I went to this thread https://codedotorg.slack.com/archives/C0T0PNTM3/p1600671794018400 and it looks like the most established pattern is to be using request.country instead of request.location.country_code. @kelbyhawn thoughts about trying the following instead?

# remove location line in_us = request.country.to_s.casecmp?('us') next unless banner["showInternationally"] ^ in_us

@dmcavoy do you know preferred behavior when we don't know the user's country?

There are three options with the showInternationally flag:

false –– only show in the US

true –– only show outside of the US

no flag –– show everywhere

There are three options with the showInternationally flag:

false –– only show in the US

true –– only show outside of the US

no flag –– show everywhere

oh interesting! how is "show everywhere" implemented? the behavior I'm seeing makes me think that "no flag" will behave like false:

irb(main):001:0> nil ^ false => false irb(main):002:0> nil ^ true => true

If we don't know a users country I think it would be good to default to the US banner for now as thats our primary audience

Oh yikes, Dave! Good call –– we tested this manually, but I think didn't cover all the cases.

@kelbyhawn I think either (a) we pair again to get this working, or (b) I pull down the branch and make the commit myself. Do you have a preference?

@megcrenshaw , given the need for thorough validation on our side, I'd encourage you to pull down the branch and make any changes needed, rather than going through Kelby for each update.

I might go ahead and add unit tests for this part too –– it will help me believe it's working

That works for me @megcrenshaw, and thanks @davidsbailey for reviewing this!

megcrenshaw · 2023-04-19T14:18:13Z

Just updated this file with refactored logic and some tests –– @breville and @davidsbailey would love for y'all to take a look when you're able! @kelbyhawn tagging you just to give you an update on progress.

kelbyhawn · 2023-04-19T15:14:29Z

      # If the banner has an array of languages, then the current language must be one of them.
      next if banner["languages"] && !banner["languages"].include?(request.language)

+      # If the banner has an showInternationally flag,


Not blocking, very minor typo: an should be a.

davidsbailey

Super glad to see this go out with test coverage. ⚓ ship it!

davidsbailey · 2023-04-19T16:30:00Z

+    banner = Homepage.get_announcement_for_page("homepage", @request)
+    assert_equal("show-everywhere", banner[:id])
+  end
+end


these tests are awesome, thanks for adding these!

I feel bad even mentioning this, but as part of the effort to separate dashboard from pegasus, we are trying to avoid cross-project dependencies like this, and so in the future any tests on pegasus pages should go in pegasus/test. this is the ideal dependency graph:

this is my bad for not setting up linting rules preventing this kind of cross-project dependency, which I have been meaning to do for some time now. so for now I'm just going to ask that any additional tests for pegasus code be located in pegasus/test. 🙏

Oh good call –– @kelbyhawn can I go ahead and update this? I hate shipping code that needs a refactor 😅

@megcrenshaw yes, absolutely! How much time do you think this'll add before it can be merged? (Only asking due to the AI banner dependency 🙃)

Give me like 15 minutes –– I have it moved with tests passing, but cleaning up a few things. Then it needs to go through drone ... so end of day your time

Just pushed new stuff. @davidsbailey do you want to take another look? It's my first rodeo writing a pegasus unit test, but the test structure didn't change. Perhaps if there are requested refactors, I can tackle them in a separate PR

davidsbailey · 2023-04-19T16:30:40Z

+      # If the banner has a showInternationally flag,
+      # only show if exactly one of showInternationally or location_unknown_or_usa are true
+      location_unknown_or_usa = request.country.nil? || request.country.to_s.casecmp?('us')
+      next if banner.key?("showInternationally") && !(banner["showInternationally"] ^ location_unknown_or_usa)


davidsbailey

thanks for moving these tests Meg! LGTM once drone passes.

I do have one question: which bucket do we fall into when viewing these pages in local development? I don't think the answer should block this PR, but worth considering what we want the local experience to be.

kelbyhawn · 2023-04-20T19:31:17Z

@megcrenshaw feel free to merge this w/o me when it's ready (unless it's better practice that I do it?)

megcrenshaw · 2023-04-20T19:38:23Z

@megcrenshaw feel free to merge this w/o me when it's ready (unless it's better practice that I do it?)

Ah, good call! It's ready. I'll merge 🥳

megcrenshaw · 2023-04-20T19:40:07Z

thanks for moving these tests Meg! LGTM once drone passes.

I do have one question: which bucket do we fall into when viewing these pages in local development? I don't think the answer should block this PR, but worth considering what we want the local experience to be.

@davidsbailey great question. When Kelby and I tested this locally, the country comes back as RD (reserved). What do you see are potential options here? Elijah had some ideas here https://codedotorg.slack.com/archives/CFTFD6BPV/p1681839473513649?thread_ts=1681767673.939399&cid=CFTFD6BPV

davidsbailey · 2023-04-20T21:06:48Z

thanks for moving these tests Meg! LGTM once drone passes.
I do have one question: which bucket do we fall into when viewing these pages in local development? I don't think the answer should block this PR, but worth considering what we want the local experience to be.

davidsbailey great question. When Kelby and I tested this locally, the country comes back as RD (reserved). What do you see are potential options here? Elijah had some ideas here https://codedotorg.slack.com/archives/CFTFD6BPV/p1681839473513649?thread_ts=1681767673.939399&cid=CFTFD6BPV

my input would just be to pick what you want the behavior to be, and have your code provide that behavior when the country code is RD. so, if you want it to give the US behavior, then location_unknown_or_usa = request.country.nil? || request.country.to_s.casecmp?('us') || .... ('RD')

davidsbailey · 2023-04-20T21:15:02Z

it sounds like this is time-sensitive, so feel free to do in a follow-up PR.

megcrenshaw · 2023-04-21T12:48:53Z

@kelbyhawn do you want the local setup to behave like the US? If so, I'll create a followup PR to address that.

kelbyhawn · 2023-04-21T15:20:38Z

@kelbyhawn do you want the local setup to behave like the US? If so, I'll create a followup PR to address that.

@megcrenshaw yes, that would be great thanks!

show pl banner for en only

af9eb26

kelbyhawn requested review from a team and dmcavoy April 14, 2023 19:44

kelbyhawn mentioned this pull request Apr 14, 2023

PL Launch 2023 - Homepage hero banner #49588

Merged

megcrenshaw reviewed Apr 17, 2023

View reviewed changes

geolocation attempt

95cd97d

add logic to show banners internationally or in US only

3746c9b

kelbyhawn requested review from breville and davidsbailey April 17, 2023 22:37

davidsbailey reviewed Apr 18, 2023

View reviewed changes

Meg Crenshaw added 3 commits April 19, 2023 09:47

add showInternationally flag

1b2128f

Remove unneccessary class_variable_set

d97b11f

Rename file to make clear about unknown location

e4a522f

megcrenshaw force-pushed the pl-homepage-banner-intl branch from 156556d to e4a522f Compare April 19, 2023 14:14

megcrenshaw requested a review from davidsbailey April 19, 2023 14:15

kelbyhawn commented Apr 19, 2023

View reviewed changes

Fix typo

726606f

davidsbailey approved these changes Apr 19, 2023

View reviewed changes

Meg Crenshaw added 4 commits April 19, 2023 14:56

Use Minitest

95fdd15

Move test outside homepage directory

227b388

Move json files into fixtures directory

19df3ca

Change json file names

9b01739

davidsbailey approved these changes Apr 19, 2023

View reviewed changes

megcrenshaw merged commit 03ded22 into staging Apr 20, 2023

megcrenshaw deleted the pl-homepage-banner-intl branch April 20, 2023 19:38

megcrenshaw mentioned this pull request Apr 24, 2023

Local testing of homepage behaves like someone in the US #51481

Merged

8 tasks

kelbyhawn mentioned this pull request May 12, 2023

PL Launch 2023 - Show homepage PL superhero banner to English only #51862

Merged

kelbyhawn changed the title ~~Pl Launch 2023 - Show default homepage hero banner to int'l audiences~~ PL Launch 2023 - Show default homepage hero banner to int'l audiences May 12, 2023

	# We have a banner. Add the ID to the hash that we return.
	return banner.merge({id: banner_id_for_page})

Uh oh!

Conversation

kelbyhawn commented Apr 14, 2023 • edited by megcrenshaw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing story

Links

Uh oh!

megcrenshaw left a comment

Choose a reason for hiding this comment

Uh oh!

dmcavoy commented Apr 17, 2023

Uh oh!

kelbyhawn commented Apr 17, 2023

Uh oh!

kelbyhawn commented Apr 17, 2023

Uh oh!

megcrenshaw commented Apr 17, 2023

Uh oh!

dmcavoy commented Apr 17, 2023

Uh oh!

kelbyhawn commented Apr 17, 2023

Uh oh!

megcrenshaw commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcavoy Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megcrenshaw commented Apr 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

kelbyhawn commented Apr 20, 2023

Uh oh!

megcrenshaw commented Apr 20, 2023

Uh oh!

megcrenshaw commented Apr 20, 2023

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

davidsbailey commented Apr 20, 2023

Uh oh!

megcrenshaw commented Apr 21, 2023

Uh oh!

kelbyhawn commented Apr 21, 2023

kelbyhawn commented Apr 14, 2023 •

edited by megcrenshaw

Loading

megcrenshaw commented Apr 17, 2023 •

edited

Loading

dmcavoy Apr 18, 2023 •

edited

Loading