Make lesson extras flag match bubble choice behavior by cforkish · Pull Request #39733 · code-dot-org/code-dot-org

cforkish · 2021-03-25T23:25:31Z

while investigating LP-1512, i found four distinct bugs around our lesson extras flag.

1

currently, the "lesson extras flag" in the header on our level pages does not update its appearance based on student progress. rather, it simply displays as green on the lesson extras (bonus) level selection page, and grey everywhere else. this PR updates it to more closely match the behavior of the header bubble for a bubble choice level.

the new behavior is that when there is no progress, it displays as small and grey when on another level in the lesson, and large and charcoal when on the bonus level select page and on the actual level page for a bonus level. when there is progress, it displays as small and green when on another level, and large and green on the level select / bonus level pages.

before:
other level

level select

bonus level

after:
other level -- no progress

level select / bonus level -- no progress

other level -- with progress

level select / bonus level -- with progress

2

currently, we cache bonus level summaries on a script instance, and when summarizing the bonus levels, we pass in the current user to get a user-specific summary. however, our script instances are cached per EC2 instance, which means that when we retrieve bonus level summaries, they include user-specific data for whatever user first viewed those bonus levels on that EC2 instance! consequently, every time the page is reloaded it will show the data cached on the EC2 instance that our load balancer routed to.

this results in very unexpected behavior, because each page reload will potentially show different progress, and occasionally even a different language!

this PR updates it so we don't include user data in the cached summaries, and instead add it per request.

3

currently, we pass current_user into the level summarization method instead of the user id included in the request. consequently we would always show a teacher's progress even when they're viewing as a student. this bug was obscured by bug 2 on production, and is fixed by the solution to same. however, this PR still updates our calls to next_level_number_for_lesson_extras and next_level_path_for_lesson_extras to pass the correct user.

4

our LessonExtrasProgressBubble component appends any current query params to the url to preserve section and user selection. however, the route for a specific bonus level page includes the level id in the params, which means that the flag icon in the header will start to preserve the level selection once the user has navigated to a specific level, making it impossible to get back to the level select page (without modifying the url). this PR updates that component to remove the level id from the query params that it appends.

Links

jira ticket: LP-1512

there are links to four related zendesk tickets in the jira ticket.

jamescodeorg

Thanks for the detailed PR description!

jamescodeorg · 2021-03-31T20:44:26Z

+    const {isLessonExtras, levels, currentLevelId} = this.props;
+    return (
+      isLessonExtras ||
+      !!levels.find(level => level.id === currentLevelId && level.bonus)


Is there always at most 1 bonus level per lesson?

no. why do you ask? this is checking that we're on the level select page (isLessonExtras), or that the current level is a bonus level (level.id === currentLevelId && level.bonus). i can add a comment to explain that.

Never mind, I think I just misread this.

!!levels.find could also be written as levels.some here

jamescodeorg · 2021-03-31T21:25:49Z

    script_data = script.summarize_header
    lesson_group_data = script.lesson_groups.map(&:summarize)
-    stage_data = (@stage || script_level.lesson).summarize
+    stage_data = (@stage || script_level.lesson).summarize(true)


Why is this change needed?

Also, very minor, including the param name here might help readability.

that true is for the include_bonus_levels param, but i couldn't figure out how to include the param name. i figured it out now, and imo summarize(include_bonus_levels = true) explains the need for the change, but lmk if you think i should also add a comment.

Sorry, I'm still not clear what's going on here, probably because I don't have a clear picture of where stage_data ends up getting used. Where does the information about the bonus levels eventually get used? What happened before when we didn't have this information?

that data is passed down to the LessonProgress component used in the header. previously, since we didn't indicate user progress for bonus levels in the header, we didn't need any level-specific data. the flag was conditionally displayed based on the presence of lessonExtrasUrl (still true), and it was colored green if isLessonExtras (now that value is only used to determine if lesson extras is "selected"). to enable the flag to be colored based on user progress, we now need to include the level-specific data for the bonus levels. this is handled in LessonProgress.isOnBonusLevel() and LessonProgress.isBonusComplete()

jamescodeorg · 2021-03-31T21:30:15Z

+        user_level = UserLevel.find_by(
+          level_id: level_summary[:level_id], user_id: user.id, script: @script
+        )
+        level_summary[:perfect] = user_level ? true : false


Not sure if this change was intentional or not, but the true case used to return user_level&.perfect?

that call is redundant because user_level != nil === user_level.perfect? == true, but level_summary[:perfect] = user_level&.perfect? seems clearer, so i will update

jamescodeorg · 2021-03-31T21:31:55Z

    @script = @stage.script
+    script_bonus_levels_by_stage = @script.get_bonus_script_levels(@stage)
+
+    # get user progress for bonus levels


I think it'd be good to elaborate on this comment a bit and indicate why we're doing this here. Also, this loop now also sets the localized display name so may want to move/adjust this comment.

maureensturgeon · 2021-04-01T16:34:07Z

  }

+  isBonusComplete() {
+    return !!this.props.levels.find(


This could also be written as return this.props.levels.some(level => level.bonus && level.status === LevelStatus.perfect)

cforkish force-pushed the cforkish/LP-1512-fix-lesson-extras-flag branch 2 times, most recently from 1c4a4b4 to c561625 Compare March 26, 2021 22:50

Charlie Forkish added 3 commits March 31, 2021 11:31

update lesson extras flag behavior in header to match BubbleChoice

24fe1bb

fix showing progress for teacher when viewing lesson extras as student

9bc10f2

add tests to cover leaking progress fix

c75617c

cforkish force-pushed the cforkish/LP-1512-fix-lesson-extras-flag branch from 9948d37 to f28d882 Compare March 31, 2021 18:32

stop storing user data in cached summary of script bonus levels

0d2bf2b

cforkish force-pushed the cforkish/LP-1512-fix-lesson-extras-flag branch from f28d882 to 0d2bf2b Compare March 31, 2021 19:03

cforkish requested review from a team and jamescodeorg March 31, 2021 19:07

jamescodeorg reviewed Mar 31, 2021

View reviewed changes

cleanup based on PR feedback

c7e7abd

maureensturgeon reviewed Apr 1, 2021

View reviewed changes

maureensturgeon approved these changes Apr 1, 2021

View reviewed changes

Charlie Forkish added 2 commits April 1, 2021 17:35

fix test, minor cleanup

88b85b6

trigger drone run

f1a84e7

cforkish merged commit 5ca40e1 into staging Apr 2, 2021

cforkish deleted the cforkish/LP-1512-fix-lesson-extras-flag branch April 2, 2021 21:28

cforkish mentioned this pull request Apr 2, 2021

Revert "Make lesson extras flag match bubble choice behavior" #39880

Merged

cforkish restored the cforkish/LP-1512-fix-lesson-extras-flag branch April 12, 2021 19:45

cforkish mentioned this pull request Apr 12, 2021

remove user data from cached bonus level summaries #40035

Merged

cforkish pushed a commit that referenced this pull request Apr 12, 2021

squash of PR #39733 minus PR #40035

052a78e

cforkish mentioned this pull request Apr 12, 2021

Make lesson extras flag match bubble choice behavior #2 #40036

Merged

cforkish pushed a commit that referenced this pull request May 11, 2021

rebased squash of PR #39733 minus PR #40035

7667320

Conversation

cforkish commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1

2

3

4

Links

Uh oh!

jamescodeorg 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cforkish commented Mar 25, 2021 •

edited

Loading