Update user_progress and user_app_options to support teacher view for cached levels #43422

jamescodeorg · 2021-11-05T21:56:07Z

This change updates ApiController#user_app_options to take a user_id, verify that the signed-in user is allowed to see the requested user progress, and returns the requested user's data. The diff doesn't do a great job of capturing the actual changes. Most of the logic in user_app_options is pre-existing with the following changes:

Moved the code to get progress-related app_options to a separate method.
Stopped sending progress and isHoc fields in the response (since they weren't being used on the client).
Started sending isStarted, skipInstructionsPopup, readonlyWorkspace, and callouts

Some additional small changes:

Pass through user_id in both the user_progress and user_app_options calls on the client.
Handle new values returned by user_app_options on the client.
Removed slogging for activity_start and activity_finish which are no longer needed (slack).
Update ApiController#user_progress to check that signed-in user can see requested user's progress.

With these changes, teacher view works for cached levels if you comment out the line that returns the "Student code cannot be viewed for this activity."

Links

jira ticket: LP-2086

Testing story

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

apps/src/code-studio/initApp/loadApp.js

maureensturgeon

Looks good! Just a couple small comments

apps/src/code-studio/progress.js

jamescodeorg · 2021-11-09T17:10:41Z

apps/test/unit/code-studio/initApp/loadAppTest.js

-        `/api/example_solutions/${serverScriptLevelId}/${serverLevelId}?section_id=`
-      )
-      .returns(exampleSolutionsResponse);
+    ajaxStub.onCall(0).returns(userAppOptionsResponse);


Path-specific stubbing wasn't working because different tests use different query parameters. This is a temporary fix so that we can get this merged and unblock the next set of work. If it's ok with you, I'd like to merge this and then clean-up these tests in a follow-up PR.

… cached levels

maureensturgeon · 2021-11-10T18:50:16Z

dashboard/app/controllers/activities_controller.rb

@@ -131,16 +131,6 @@ def milestone
      user_level: @user_level
    )

-    if solved


Just curious what this was for?

This was for the "activity monitor". See this slack thread for more info on why it's no longer needed.

maureensturgeon

LGTM! Just a couple of questions

maureensturgeon · 2021-11-10T18:53:15Z

dashboard/app/controllers/api_controller.rb

@@ -425,8 +425,14 @@ def script_standards
  # Return a JSON summary of the user's progress for params[:script].
  def user_progress
    if current_user
+      if params[:user_id].present?
+        user = User.find(params[:user_id])


Should we guard against ActiveRecord::RecordNotFound here? Or replace with find_by or some other method which will return nil if record couldn't be found?

Good question. It's intentional that we throw an exception here so we don't continue execution here (same as the previous behavior). As written, if the request has an invalid user_id, the whole request will fail with a 404 error. I'm not sure if this is the best error to return and we also don't really handle this error on the client but this behavior is pre-existing and I'm inclined to solve that problem another day?

maureensturgeon · 2021-11-10T18:54:32Z

dashboard/app/controllers/api_controller.rb

+      # and note if the signed-in user is viewing another user (e.g. a teacher
+      # viewing a student's work).
+      if params[:user_id].present?
+        user = User.find(params[:user_id])


Same question here about checking for ActiveRecord::RecordNotFound

maureensturgeon · 2021-11-10T18:58:48Z

dashboard/app/controllers/api_controller.rb

+      # TODO: There are many other user-specific values in app_options that may
+      # need to be sent down.  See LP-2086 for a list of potential values.
+
+      response[:disableSocialShare] = !!user.under_13?


I'm curious why the !! is needed, under_13? seems to imply it returns a boolean from the ?

Oh, good catch. I wonder why I did that. :-)

I'll pick up this change in my next PR.

maureensturgeon · 2021-11-10T19:08:54Z

dashboard/test/controllers/api_controller_test.rb

-
-    user = create :user
-    sign_out user
+  test "user_app_options should normally return reduceChannelUpdates false" do


Nit: "normally" could be clarified (not in emergency mode?)

jamescodeorg requested a review from a team November 5, 2021 22:17

maureensturgeon reviewed Nov 9, 2021

View reviewed changes

apps/src/code-studio/initApp/loadApp.js Show resolved Hide resolved

maureensturgeon reviewed Nov 9, 2021

View reviewed changes

apps/src/code-studio/progress.js Show resolved Hide resolved

jamescodeorg force-pushed the jamescodeorg/cached-level-load-source branch 2 times, most recently from 509c2cb to bdc4b21 Compare November 9, 2021 15:47

jamescodeorg commented Nov 9, 2021

View reviewed changes

jamescodeorg force-pushed the jamescodeorg/cached-level-load-source branch from bdc4b21 to 908fced Compare November 9, 2021 18:22

maureensturgeon mentioned this pull request Nov 9, 2021

Test example solutions endpoint & refactor #43470

Merged

8 tasks

jamescodeorg force-pushed the jamescodeorg/cached-level-load-source branch 2 times, most recently from b8a7977 to 368e4f3 Compare November 10, 2021 00:30

jamescodeorg added 2 commits November 9, 2021 17:04

Update user_progress and user_app_options to support teacher view for…

4282426

… cached levels

Add/update unit tests

7aa8d4d

jamescodeorg force-pushed the jamescodeorg/cached-level-load-source branch from 368e4f3 to 7aa8d4d Compare November 10, 2021 01:04

maureensturgeon reviewed Nov 10, 2021

View reviewed changes

maureensturgeon approved these changes Nov 10, 2021

View reviewed changes

jamescodeorg merged commit c8da451 into staging Nov 10, 2021

jamescodeorg deleted the jamescodeorg/cached-level-load-source branch November 10, 2021 20:24

jamescodeorg added the cached level teacher view label Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update user_progress and user_app_options to support teacher view for cached levels #43422

Update user_progress and user_app_options to support teacher view for cached levels #43422

jamescodeorg commented Nov 5, 2021 •

edited

maureensturgeon left a comment

jamescodeorg Nov 9, 2021

maureensturgeon Nov 10, 2021

jamescodeorg Nov 10, 2021

maureensturgeon left a comment

maureensturgeon Nov 10, 2021

jamescodeorg Nov 10, 2021

maureensturgeon Nov 10, 2021

maureensturgeon Nov 10, 2021

jamescodeorg Nov 10, 2021

jamescodeorg Nov 10, 2021

maureensturgeon Nov 10, 2021

Update user_progress and user_app_options to support teacher view for cached levels #43422

Update user_progress and user_app_options to support teacher view for cached levels #43422

Conversation

jamescodeorg commented Nov 5, 2021 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

maureensturgeon 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

maureensturgeon 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

jamescodeorg commented Nov 5, 2021 •

edited