Update circuit playground discount code applications for 2020 #32179

bencodeorg · 2019-11-27T20:04:51Z

Description

Updates "high needs" bar eligibility for discount codes from 40% eligible for free and reduced meals to 50%.
Updates strings used throughout the application to be appropriate for this school year.
Checks for a fourth criteria (whether teacher has attended Q4 workshop) and provides that check as one of the elements in the "eligibility checklist". Makes model updates to account for this change.
Allows for teachers to get either a full discount code with or without shipping (with shipping codes to be used for Alaska and Hawaii partners). The approach I'm proposing here is that the full_discount flag on CircuitPlaygroundDiscountApplication would be 0 if a teacher is getting a discount code without shipping, and 1 if they are getting one with shipping (this may be inconsistent with last year, if "full discount" discount codes last year were without shipping).
Updates "Unit 6 Validation" step to language specified by Programs team.
Updates appropriate tests to account for changes.

Known issues:
Need to add tests to account for new features (namely, check on attendance at Q4 workshop).

Links

jira

Testing story

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

…ligibility

dashboard/test/models/circuit_playground_discount_application_test.rb

dashboard/app/models/circuit_playground_discount_application.rb

bencodeorg · 2019-12-11T00:54:55Z

@islemaster before I make some changes to allow this to work correctly for AK/HI teachers, could use your two cents on this point:

4. Allows for teachers to get either a full discount code with or without shipping (with shipping codes to be used for Alaska and Hawaii partners). The approach I'm proposing here is that the full_discount flag on CircuitPlaygroundDiscountApplication would be 0 if a teacher is getting a discount code without shipping, and 1 if they are getting one with shipping (this I think is inconsistent with last year, if "full discount" discount codes last year were without shipping).

Do you care if there's inconsistency there in whether this year full discount (without shipping) is represented as 0 for full_discount, and last year was represented as 1?

I could change the column type and make it something like this, but it would be nice to keep it as a boolean, methinks.

islemaster · 2019-12-11T18:01:18Z

I'm not worried about year-over-year inconsistency at all, as long as we think it's documented and understandable for someone to revisit next year. IMO full_discount meaning "with shipping" makes sense in isolation and would be fine. All codes for previous years expire on December 31st and I don't feel any need to keep the code for them in a working state.

bencodeorg · 2019-12-11T20:34:06Z

@islemaster I dug in a bit this morning, and I'm worried that the 0/1 full_discount is too intertwined with the idea of whether a school meets the "maker high needs" bar. For example, we set the full_discount value initially based on the teacher's school characteristics. As a result, a teacher who meets the high needs bar would also have full_discount set to 1, making them eligible for the "with shipping" codes in the approach I suggested ^^^.

The options I see are:

(more work) Separate the idea of full_discount from the idea of school_eligibility.
(less work, but maybe too hacky) Keep full discount codes (no shipping) as full_discount = 1 (the case that most teachers will face). Make discount codes with shipping as full_discount = 0. Only admins would be able to get teachers into this state.

I'm leaning towards option 2. Thoughts?

islemaster · 2019-12-11T21:29:03Z

I'd still lean toward option 1. I don't think we read full_discount anywhere to determine whether a teacher is eligible to receive a code at all. We set it there based on some old logic, but I think we could update that one line to initially set it based on the school's state instead, without breaking anything else.

Happy to sit down and look at this together tomorrow, if you'd like.

apps/src/lib/kits/maker/ui/EligibilityChecklist.jsx

islemaster · 2019-12-13T20:07:28Z

apps/src/lib/kits/maker/ui/EligibilityChecklist.story.jsx

@@ -72,7 +73,7 @@ export default storybook => {
        )
      },
      {


Let's add stories to this file for the scenario where the academic year PD requirement has not been passed.

dashboard/app/models/circuit_playground_discount_application.rb

islemaster · 2019-12-13T20:20:05Z

dashboard/app/models/circuit_playground_discount_application.rb

@@ -54,11 +53,11 @@ def self.find_by_studio_person_id(studio_person_id)
  end

  # @return {boolean} true if user is an eligible facilitator or attended relevant workshop


Please update this doc comment to explain the new workshop_subjects parameter.

Updated -- I'm not familiar with YARD yet (which is looks like is what we use for our docs?), so hopefully this is syntactically ok.

👍 Seems fine. We're not actually generating these docs, so syntax errors in YARD are not a big deal right now.

islemaster · 2019-12-13T20:21:44Z

dashboard/app/models/circuit_playground_discount_application.rb

@@ -54,11 +53,11 @@ def self.find_by_studio_person_id(studio_person_id)
  end

  # @return {boolean} true if user is an eligible facilitator or attended relevant workshop
-  def self.user_pd_eligible?(user)
-    user_pd_eligible_as_teacher?(user) || user_pd_eligible_as_facilitator?(user)
+  def self.user_pd_eligible?(user, workshop_subjects=nil)


Why did you choose a nil default for this parameter? That passes through to the query below and would match workshops with a NULL subject, which doesn't make sense to me as a default. If it's possible to simply remove the default and require this parameter, I'd prefer that.

I made it nil by default so you could still pass in a facilitator's user ID and get a returned value of true, something like this.

I didn't consider the impact of matching a workshop with a null subject though.

Did a quick scan and it looks like we don't ever call this method without a second argument in production code, only in test code. Let's remove the default value and any tests covering that behavior.

Sounds good -- for the record, I think the link I put here wasn't to the right test, more like the one titled "studio_person_pd_eligible? returns true if studio_person_id associated User is approved facilitator" is relevant.

Updated -- I kept most tests for facilitators in place, just added the 5-day workshop as the workshop_subjects argument. I think this tests what we'd see in the wild (facilitators going through the normal flow with no enrollments associated with their accounts, but with their user ID in the list of facilitators).

👍 Great!

islemaster · 2019-12-13T20:22:57Z

dashboard/app/models/circuit_playground_discount_application.rb

@@ -111,13 +112,25 @@ def self.application_status(user)
      has_confirmed_school: application.try(:has_confirmed_school?) || false,
      school_id: school_id,
      school_name: school_id ? School.find(school_id).name : nil,
+      school_high_needs_eligible: School.find(school_id).try(:maker_high_needs?),


I liked your proposal to move the maker_high_needs? implementation out of the School model and into this one.

Alternatively, we could create a policy object, a clean-architecture Ruby pattern @Hamms introduced into our project this year.

I left this as-is for now -- could fix after break if we want.

islemaster · 2019-12-13T20:24:13Z

dashboard/app/models/circuit_playground_discount_application.rb

+          Pd::Workshop::SUBJECT_CSD_WORKSHOP_6,
+          Pd::Workshop::SUBJECT_CSD_VIRTUAL_8
+        ]
+      ),


I wonder if we can deduplicate these 12 lines with the matching ones in self.application_status?

I put together a functional approach to to this (create a method that takes a user and creates a two element hash with keys is_pd_eligible and is_quarterly_workshop_pd_eligible, then merge that hash with the one created here). It seemed kind of funky / confusing though, is there a cleaner way to do that?

islemaster · 2019-12-13T20:34:37Z

I think this is really close! Great work on this Ben. Once you've got the new full-discount logic done, I've got a few additional notes (above) to address.

As follow-up work, after we merge this PR, we should ask whether the "everyone" discount code that is hard-coded into the system is still active this year, and whether we need that list of eligible facilitators at all (or if we can just remove that code).

After all that, it might be interesting/useful to review the similar update to discount codes last year (#26382) and some fixes that happened not long after (#26823).

dashboard/test/controllers/maker_controller_test.rb

Co-Authored-By: Brad Buchanan <brad@code.org>

islemaster

This looks great Ben! Next steps:

Let's go ahead and change the base branch for this PR to staging now that it's open again.
Let's make sure we get a passing Drone run after the base branch change.
I think we can merge this in before break, and do the new changes Amy brought to our attention in a follow-up PR before we re-enable this feature in January.

Thanks for all the attention to detail on this!

bencodeorg · 2019-12-17T21:38:16Z

Sounds good, going to shoot to clean this up by EOD tomorrow.

bencodeorg added 8 commits November 11, 2019 21:13

Set high needs bar to 50%

b127918

Merge branch 'staging' into update-circuit-playground-discount-codes

8efab7e

Non-i18n String changes for 19-20 Adafruit discount codes

21413d9

More i18n updates for Adafruit discount codes

4880ad5

Add option for discount code with shipping to Alaska and Hawaii

310e549

Correct logic for discount codes with and without shipping

3cc3cd3

Check for teacher attendance at Q4 workshop

8e8fdba

Model changes to check for Q4 workshop attendance for discount code e…

21e19ac

…ligibility

bencodeorg requested a review from a team as a code owner November 27, 2019 20:04

bencodeorg requested a review from islemaster November 27, 2019 21:56

bencodeorg added 5 commits December 2, 2019 20:42

Check Unit 6 intention appropriately

26d49b4

Consistent variable naming in discount application model

278ebd3

Make long string prettier

0c9ea08

Update tests to cover 2020 discount code application requirements

6c0535d

Update storybook components for 2020 discount codes

0402a4d

bencodeorg commented Dec 3, 2019

View reviewed changes

dashboard/test/models/circuit_playground_discount_application_test.rb Show resolved Hide resolved

bencodeorg added 5 commits December 3, 2019 15:46

Update apps tests

8e8e0e2

More apps test updates for 2020 discount codes

411d349

Update discount logic apps test

7d03566

Fix more dashboard discount code tests

a2b4791

Fix school tests to account for new high needs bar

1d89206

bencodeorg changed the base branch from staging to staging-next December 6, 2019 18:13

Fix enum in circuit playground discount application

de64b40

bencodeorg commented Dec 6, 2019

View reviewed changes

dashboard/app/models/circuit_playground_discount_application.rb Outdated Show resolved Hide resolved

Attendance at virtual workshop 8 counts for discount eligibility

c2d691d

Add string to alert AK/HI teachers about shipping

92d8727

Separate discount code type from school eligibility

3a8e9e6

islemaster reviewed Dec 13, 2019

View reviewed changes

apps/src/lib/kits/maker/ui/EligibilityChecklist.jsx Outdated Show resolved Hide resolved

islemaster reviewed Dec 13, 2019

View reviewed changes

dashboard/app/models/circuit_playground_discount_application.rb Show resolved Hide resolved

islemaster reviewed Dec 13, 2019

View reviewed changes

bencodeorg added 3 commits December 13, 2019 15:22

Give full discount to teachers in Alaska and Hawaii

2f858df

Remove unnecessary new language about AK/HI

e573d65

Keep old options in enum for Unit 6 intention responses

3568e5c

islemaster reviewed Dec 16, 2019

View reviewed changes

dashboard/test/controllers/maker_controller_test.rb Outdated Show resolved Hide resolved

Use refute instead of assert_equal for false test

ef1c2ea

Co-Authored-By: Brad Buchanan <brad@code.org>

islemaster approved these changes Dec 17, 2019

View reviewed changes

bencodeorg changed the base branch from staging-next to staging December 17, 2019 21:58

bencodeorg changed the title ~~[WIP] Update circuit playground discount codes~~ Update circuit playground discount code applications for 2020 Dec 17, 2019

bencodeorg added 9 commits December 18, 2019 15:23

Require workshop subject when checking PD eligibility

b55117b

String update for 2020

58d0f22

Merge branch 'staging' into update-circuit-playground-discount-codes

68fbb17

Pull commit made via GitHub UI

9a40301

Add documentation to check on PD attendance method

a7d307b

Update storybooks and JS unit tests

0b53924

Update factories and tests for maker controller updates

1c4d7a5

Allow application status check for application with no school

0098b70

Merge staging

582465b

bencodeorg merged commit dba7401 into staging Dec 21, 2019

bencodeorg deleted the update-circuit-playground-discount-codes branch December 21, 2019 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update circuit playground discount code applications for 2020 #32179

Update circuit playground discount code applications for 2020 #32179

bencodeorg commented Nov 27, 2019 •

edited

bencodeorg commented Dec 11, 2019 •

edited

islemaster commented Dec 11, 2019

bencodeorg commented Dec 11, 2019

islemaster commented Dec 11, 2019

islemaster Dec 13, 2019

bencodeorg Dec 20, 2019

islemaster Dec 13, 2019

bencodeorg Dec 19, 2019

islemaster Dec 19, 2019

islemaster Dec 13, 2019

bencodeorg Dec 14, 2019

islemaster Dec 16, 2019

bencodeorg Dec 17, 2019

bencodeorg Dec 19, 2019

islemaster Dec 19, 2019

islemaster Dec 13, 2019

islemaster Dec 13, 2019

bencodeorg Dec 20, 2019

islemaster Dec 13, 2019

bencodeorg Dec 20, 2019

islemaster commented Dec 13, 2019

islemaster left a comment

bencodeorg commented Dec 17, 2019

@@ @@ -72,7 +73,7 @@ export default storybook => { @@
                       )
                     },
                     {

		@@ -54,11 +53,11 @@ def self.find_by_studio_person_id(studio_person_id)
		end

		# @return {boolean} true if user is an eligible facilitator or attended relevant workshop

Update circuit playground discount code applications for 2020 #32179

Update circuit playground discount code applications for 2020 #32179

Conversation

bencodeorg commented Nov 27, 2019 • edited

Description

Links

Testing story

Reviewer Checklist:

bencodeorg commented Dec 11, 2019 • edited

islemaster commented Dec 11, 2019

bencodeorg commented Dec 11, 2019

islemaster commented Dec 11, 2019

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

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

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

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 commented Dec 13, 2019

islemaster left a comment

Choose a reason for hiding this comment

bencodeorg commented Dec 17, 2019

bencodeorg commented Nov 27, 2019 •

edited

bencodeorg commented Dec 11, 2019 •

edited