Show PL Program Info Page even without Programs Offered by TurnerRiley · Pull Request #49421 · code-dot-org/code-dot-org

TurnerRiley · 2022-12-09T18:31:46Z

A bug on the Professional Learning Program Information page was discovered where no information was being shown for regional partners not offering a program at that time (which was throwing an error in console). It's a bug from this PR where it would try to load the workshop cards but not find any programs offered.

This PR just adds a check to make sure there are programs before trying to render cards for them. If there are no programs offered, it will just show the rest of the information without the workshop cards.

Bug behavior for no programs offered

With fix behavior for no programs offered

Links

Slack thread this was brought up: here

Testing story

Local testing.

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
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

TurnerRiley · 2022-12-09T18:32:42Z

+                      // If no current workshops for a course
+                      if (
+                        partnerInfo.pl_programs_offered.includes(collection.key)
+                      ) {


This .includes(...) was where the error was previously being thrown since pl_programs_offered was null

Thanks for pointing this out

Wait, why would this run if pl_programs_offered is null? We're in a

appState !== WorkshopApplicationStates.now_closed && partnerInfo.pl_programs_offered

condition.

And if it is run, should we use optional chaining? I.e. partnerInfo.pl_programs_offered?.includes()

Ahh, I see –– you added that conditional up top

It should not run if it's null, it was previously throwing an error calling "includes" on a null object. What is optional chaining?

Okay sorry –– should've waited until I fully understood the logic.

No worries at all :)

megcrenshaw

Thanks for addressing this! I have a questions and some ideas for refactors:

Should pl_programs_offered or summer_workshops dictate what gets shown? We're using both here in different places and I don't understand why. It looks like pl_programs_offered relies on the RP correctly filling out their info on their Regional Partner page, whereas summer_workshops we get by querying our workshops. I'm not sure what we should rely on (and it might be a product question).
We have conditional logic in both workshopCollections and partnerInfo –– for readability, can you consolidate this logic so that it lives in one place? This is related to the attributes named in (1).
I should've caught this the first time, but I'm reading the workshopCollections and am getting confused by the information it contains. Could you consider renaming some things: workshopCollections, collections, and key aren't helping me figure out what information lives here.
I think some of the logic here could be consolidated and/or simplified by using Optional Chaining. Instead of
partnerInfo && partnerInfo.summer_workshops.filter()
It could be partnerInfo?.summer_workshops?.filter() which covers BOTH if partnerInfo is nil and if summer_workshops is nil. Another option is to set some default values so that you don't need to keep checking these.
I made a test file recently to ensure the StartApplicationButton was showing at the right times. I don't think you need to add tests for everything in this whole file (although that would be great!), but since the test file now exists, could you add unit tests for the logic you've added recently?
How urgent is this? I'm in favor of simplifying the logic and adding tests so we can avoid future needs for quick-fixes, but if we're getting lots of reports on this, I can be swayed towards something else.

megcrenshaw · 2022-12-09T22:32:10Z

@dmcavoy could you chime in on questions 1 and 5 above?

TurnerRiley · 2022-12-09T22:39:54Z

Thanks for addressing this! I have a questions and some ideas for refactors:

Should pl_programs_offered or summer_workshops dictate what gets shown? We're using both here in different places and I don't understand why. It looks like pl_programs_offered relies on the RP correctly filling out their info on their Regional Partner page, whereas summer_workshops we get by querying our workshops. I'm not sure what we should rely on (and it might be a product question).

We have conditional logic in both workshopCollections and partnerInfo –– for readability, can you consolidate this logic so that it lives in one place? This is related to the attributes named in (1).

I should've caught this the first time, but I'm reading the workshopCollections and am getting confused by the information it contains. Could you consider renaming some things: workshopCollections, collections, and key aren't helping me figure out what information lives here.

I think some of the logic here could be consolidated and/or simplified by using Optional Chaining. Instead of
partnerInfo && partnerInfo.summer_workshops.filter()
It could be partnerInfo?.summer_workshops?.filter() which covers BOTH if partnerInfo is nil and if summer_workshops is nil. Another option is to set some default values so that you don't need to keep checking these.

I made a test file recently to ensure the StartApplicationButton was showing at the right times. I don't think you need to add tests for everything in this whole file (although that would be great!), but since the test file now exists, could you add unit tests for the logic you've added recently?

How urgent is this? I'm in favor of simplifying the logic and adding tests so we can avoid future needs for quick-fixes, but if we're getting lots of reports on this, I can be swayed towards something else.

Thank you so much for all the feedback! Yes I will get on these changes! I don't think this is that urgent so I can start refactoring and cleaning up!

dmcavoy · 2022-12-12T15:02:49Z

Should pl_programs_offered or summer_workshops dictate what gets shown? We're using both here in different places and I don't understand why. It looks like pl_programs_offered relies on the RP correctly filling out their info on their Regional Partner page, whereas summer_workshops we get by querying our workshops. I'm not sure what we should rely on (and it might be a product question).

So we need both to make the decisions.

No Programs offered -> Hide the area since this probably means the regional partner details have not been set yet.
Program
Some or all programs offered -> For programs offer if workshop has been scheduled show the workshop details otherwise show a message that the workshop details are coming soon. For the programs not offered show a message that tells them they can still apply but that we will have to figure out where to place them since their region is not offering.

Does that help?

dmcavoy · 2022-12-12T15:03:35Z

For #5 I think just adding tests for the new things that are being added makes sense.

megcrenshaw · 2022-12-12T15:25:16Z

Should pl_programs_offered or summer_workshops dictate what gets shown? We're using both here in different places and I don't understand why. It looks like pl_programs_offered relies on the RP correctly filling out their info on their Regional Partner page, whereas summer_workshops we get by querying our workshops. I'm not sure what we should rely on (and it might be a product question).

So we need both to make the decisions.

No Programs offered -> Hide the area since this probably means the regional partner details have not been set yet. Program Some or all programs offered -> For programs offer if workshop has been scheduled show the workshop details otherwise show a message that the workshop details are coming soon. For the programs not offered show a message that tells them they can still apply but that we will have to figure out where to place them since their region is not offering.

Does that help?

Got it, thanks! Sounds good.

Turner, since both are being used, I think it'd be most readable to access these values using similar logic (either by setting both in the collections object, or accessing them directly using the regionalPartnerInfo prop).

megcrenshaw · 2022-12-14T22:57:43Z

+        heading: `${ActiveCourseWorkshops[courseKey]} Workshops`,
+        isOffered: partnerInfo?.pl_programs_offered?.includes(courseKey),
+        summerWorkshops: partnerInfo?.summer_workshops?.filter(
+          workshop => workshop.course === ActiveCourseWorkshops[courseKey]


LOVE this structure using ActiveCourseWorkshops to push!

And the optional chaining 🙌

megcrenshaw · 2022-12-14T23:05:32Z

+  workshop_date_range_string: 'Test CSD workshop dates',
+  location_name: 'Test CSD workshop location',
+  location_address: 'Test CSD workshop address'
+};


This is great!

megcrenshaw · 2022-12-14T23:08:07Z

+  });
+  it('shows Not Offering note on workshop card(s) for the program(s) not being offered when other programs are offered', () => {
+    createServerResponses(server, true, false, OFFERED_PROGRAMS, [
+      OFFERED_WORKSHOP


This is great! And serves as documentation on how we expect data coming in from the backend to look

megcrenshaw

This looks amazing! Nice work –– lots of weird cases to catch, and the tests document them beautifully.

…)" This reverts commit 3f42d05.

…)" (#49565) This reverts commit 3f42d05.

fix not showing info bug

d99e6d9

TurnerRiley commented Dec 9, 2022

View reviewed changes

TurnerRiley requested a review from a team December 9, 2022 18:33

megcrenshaw reviewed Dec 9, 2022

View reviewed changes

TurnerRiley added 5 commits December 12, 2022 14:20

begin cleanup of logic for readability and add test comments

add4b3d

add basic unit tests

deef45a

update tests0

b5a9fe5

finish adding tests

ea21917

fix isOffered and simplify courseWorkshops creation

7c43320

TurnerRiley changed the base branch from staging-next to staging December 14, 2022 21:30

TurnerRiley requested a review from megcrenshaw December 14, 2022 21:30

megcrenshaw reviewed Dec 14, 2022

View reviewed changes

megcrenshaw approved these changes Dec 14, 2022

View reviewed changes

TurnerRiley merged commit 3f42d05 into staging Dec 15, 2022

TurnerRiley deleted the show-RP-info-without-programs branch December 15, 2022 21:08

epeach pushed a commit that referenced this pull request Dec 15, 2022

Revert "Show PL Program Info Page even without Programs Offered (#49421…

5dd5ec6

…)" This reverts commit 3f42d05.

epeach mentioned this pull request Dec 15, 2022

Revert "Show PL Program Info Page even without Programs Offered" #49565

Merged

epeach pushed a commit that referenced this pull request Dec 15, 2022

Revert "Show PL Program Info Page even without Programs Offered (#49421…

80fa221

…)" (#49565) This reverts commit 3f42d05.

TurnerRiley restored the show-RP-info-without-programs branch December 16, 2022 01:16

TurnerRiley mentioned this pull request Dec 16, 2022

Show PL Program Info Page even without Programs Offered #49586

Merged

8 tasks

Conversation

TurnerRiley commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug behavior for no programs offered

With fix behavior for no programs offered

Links

Testing story

PR Checklist:

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 Dec 9, 2022 • 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!

megcrenshaw left a comment • 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!

megcrenshaw commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurnerRiley commented Dec 9, 2022

Uh oh!

dmcavoy commented Dec 12, 2022

Uh oh!

dmcavoy commented Dec 12, 2022

Uh oh!

megcrenshaw commented Dec 12, 2022

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TurnerRiley commented Dec 9, 2022 •

edited

Loading

megcrenshaw Dec 9, 2022 •

edited

Loading

megcrenshaw left a comment •

edited

Loading

megcrenshaw commented Dec 9, 2022 •

edited

Loading