Fix time spent on bubble choice = sum of sub-level time spent #40117

maureensturgeon · 2021-04-16T00:41:03Z

Refactor merge_user_progress_by_level (users_helper function that determines progress for each level)
Update time_spent for parent bubble choice level to be sum of sublevel time_spent

Links

Testing story

Added unit tests and manually tested a bubble choice by adding progress on multiple choice levels, then checking time spent in the progress table.

Deployment strategy

Follow-up work

Privacy

Security

Caching

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

dashboard/app/helpers/users_helper.rb

maureensturgeon · 2021-04-20T00:00:49Z

dashboard/app/controllers/api_controller.rb

@@ -342,41 +342,6 @@ def section_level_progress
    }
  end

-  # Get level progress for a set of users within this script.


Moved to users_helper

… time spent

jamescodeorg

I'm not quite done yet but sending out some questions before a long stretch of meetings.

This is an excellent improvement! One of the things I've been wondering about is how to structure all the special-case logic for calculating progress for different level types and this change really helps reveal some of the structure here.

jamescodeorg · 2021-04-20T17:53:35Z

dashboard/app/helpers/users_helper.rb

+    completion_status = activity_css_class(user_level)
+    locked = user_level.try(:show_as_locked?, script_level.lesson) || script_level.lesson.lockable? && !user_level
+
+    # if the level has not been tried, less progress data is necessary


Could you elaborate a bit more on why we need a special case for not_tried here?

Yup! So if a level status is not tried there are two possible things that happen:

If it's not locked we don't merge any progress for that level

If it is locked then we merge only status and locked state for the progress for that level

In either one of those cases we don't need a full set of progress data so we return just what we need. I'll update the comment and try to make it more clear.

jamescodeorg · 2021-04-20T17:54:38Z

dashboard/app/helpers/users_helper.rb

+      readonly_answers: !!user_level.try(:readonly_answers) || nil,
+      paired: (paired_user_levels.include? user_level.try(:id)) || nil,
+      last_progress_at: include_timestamp ? user_level&.updated_at&.to_i : nil,
+      time_spent: user_level&.time_spent&.to_i


Are .try and &. equivalent? If so, maybe we can be more consistent about using one or the other, at least within a function?

I actually had to look this up. So it sounds like the main difference is that using try, if a method is called that doesn't exist for the object then it will swallow the NoMethodError and return nil. I think moving towards & seems like the right call here. I'll update.

TIL! thanks for that distinction.

Okay so turns out Free Response levels have a peer_reviewable method but other types of levels don't so that's a valid use of try that I'll need to leave there.

cforkish

LGTM! Nicely done!

cforkish · 2021-04-21T19:40:59Z

dashboard/app/controllers/api_controller.rb

@@ -342,41 +342,6 @@ def section_level_progress
    }
  end

-  # Get level progress for a set of users within this script.


cforkish · 2021-04-21T19:44:32Z

dashboard/app/helpers/users_helper.rb

@@ -160,6 +195,8 @@ def user_summary(user)
  #   this in advance for many users in some use cases.
  # @param [Enumerable<Integer>] paired_user_levels
  #   A collection of UserLevel ids where the user was pairing.
+  # @param [Boolean] include_timestamp


are there any cases where we don't pass true for this? i can't think of a use case for that. if we're not using it, let's remove it!

true isn't passed in here https://github.com/code-dot-org/code-dot-org/blob/staging/dashboard/app/helpers/users_helper.rb#L143

hmmm. i find that questionable. from a brief inspection it looks like we only include that data for the teacher dashboard, but i can't think of a reason for that. it doesn't require any extra db queries and would result in very minor data bloat, so imo it would be worth the reduction in complexity to remove it. could you try to find out if there's a specific reason we're excluding it?

I agree we should investigate, going to punt this for now to avoid increasing the scope of this ticket

cforkish · 2021-04-21T19:46:23Z

dashboard/app/helpers/users_helper.rb

          }.compact
        end
      end
    end
    progress
  end

+  # Summarizes a user's level progress for bubble choice level (parent level and sublevels)
+  private def get_bubble_choice_progress(level, user, user_levels_by_level, script_level, paired_user_levels, include_timestamp)


NIT: in terms of figuring out how this code works, imo get_level_progress should come before this.

that said, i do think factoring this out into its own helper function really improves the readability of merge_user_progress_by_level 👍

I'll move it down!

cforkish reviewed Apr 16, 2021

View reviewed changes

dashboard/app/helpers/users_helper.rb Outdated Show resolved Hide resolved

dashboard/app/helpers/users_helper.rb Show resolved Hide resolved

cforkish reviewed Apr 16, 2021

View reviewed changes

dashboard/app/helpers/users_helper.rb Outdated Show resolved Hide resolved

maureensturgeon commented Apr 20, 2021

View reviewed changes

maureensturgeon added 5 commits April 20, 2021 09:23

refactor merge_user_progress_by_level in users_helper

d905c97

more refactor of progress merge

b36290d

additional refactor to users_helper merge_user_progress_by_level, fix…

b70aee1

… time spent

add test for merge_user_progress helper

187fcc6

add tests, move script_progress_for_users to users_helper

5c3f09f

maureensturgeon force-pushed the maureen/LP-1851-time-spent-bubble-choice branch from 46705c3 to 5c3f09f Compare April 20, 2021 16:26

maureensturgeon marked this pull request as ready for review April 20, 2021 16:27

maureensturgeon requested a review from a team April 20, 2021 16:27

jamescodeorg reviewed Apr 20, 2021

View reviewed changes

cforkish mentioned this pull request Apr 21, 2021

ProgressTableLevelBubble caching #40193

Merged

add comments and refactor .try to &

14221cf

cforkish approved these changes Apr 21, 2021

View reviewed changes

maureensturgeon added 2 commits April 21, 2021 13:03

re-order private methods in users_helper

3c69142

user helpers replace & with try for checking peer_reviewable

b238150

maureensturgeon merged commit 8c938fb into staging Apr 22, 2021

maureensturgeon deleted the maureen/LP-1851-time-spent-bubble-choice branch April 22, 2021 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix time spent on bubble choice = sum of sub-level time spent #40117

Fix time spent on bubble choice = sum of sub-level time spent #40117

maureensturgeon commented Apr 16, 2021 •

edited

maureensturgeon Apr 20, 2021

cforkish Apr 21, 2021

jamescodeorg left a comment

jamescodeorg Apr 20, 2021

maureensturgeon Apr 21, 2021 •

edited

jamescodeorg Apr 20, 2021

maureensturgeon Apr 21, 2021

cforkish Apr 21, 2021

maureensturgeon Apr 22, 2021

cforkish left a comment

cforkish Apr 21, 2021

cforkish Apr 21, 2021

maureensturgeon Apr 21, 2021

cforkish Apr 21, 2021

maureensturgeon Apr 22, 2021

cforkish Apr 21, 2021

cforkish Apr 21, 2021

maureensturgeon Apr 21, 2021

Fix time spent on bubble choice = sum of sub-level time spent #40117

Fix time spent on bubble choice = sum of sub-level time spent #40117

Conversation

maureensturgeon commented Apr 16, 2021 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamescodeorg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maureensturgeon Apr 21, 2021 • edited

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

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

maureensturgeon commented Apr 16, 2021 •

edited

maureensturgeon Apr 21, 2021 •

edited