On teacher feedback review state change, reload progress bubbles on page #41708

maureensturgeon · 2021-07-22T23:52:30Z

Creates a new endpoint to query data for teacher panel
Replaces TeacherPanelProgress bubble with ProgressBubble
When feedback review state is changed by teacher, progress is updated (for header and teacher panel progress bubbles)

Screen.Recording.2021-07-22.at.5.01.12.PM.mov

Links

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

maureensturgeon · 2021-07-30T18:22:23Z

dashboard/test/controllers/api_controller_test.rb

+    assert_response :forbidden
+  end
+
+  # test "" do


Missing test?

Ooops, will remove this.

Would you mind just removing this test for me so we can avoid having to coordinate commits?

maureensturgeon · 2021-07-30T18:34:16Z

dashboard/app/controllers/api_controller.rb

+      script_level = script.script_levels.find do |sl|
+        sl.level_ids.include?(params[:level_id].to_i)
+      end
+      return head :bad_request if script_level.nil?


Is it worth returning some kind of message describing the issue?

I'm thinking no? This would be mostly for debugging purposes and it seems like this would be easy enough to diagnose with just the error code? (famous last words)

jamescodeorg

Wow, this looks like a huge step forward!

I need to take another pass, but posting some initial comments.

jamescodeorg · 2021-08-09T17:52:21Z

apps/src/code-studio/components/progress/teacherPanel/SelectedStudentInfo.jsx

@@ -126,40 +77,48 @@ export class SelectedStudentInfo extends React.Component {
        />
        <div style={styles.studentInfo}>
          <div style={styles.name}>{selectedStudent.name}</div>
-          {userLevel.paired && (
+          {levelWithProgress?.paired && (


In what scenarios do we have / not have a levelWithProgress object? What is shown when levelWithProgress is not present? (I'm wondering whether it would make sense to split that out as a specific case because all of these elements are shown only if levelWithProgress being present.)

Now that level/progress data is being loaded async, there is a moment while data is loading when this value will be undefined. However, the student data is available on load so we can immediately show the student name which is why this component is rendered. We could wait until the level data is loaded to render this component, I don't have a strong opinion.

jamescodeorg · 2021-08-09T17:54:10Z

apps/src/code-studio/components/progress/teacherPanel/StudentTable.jsx

-    if (this.props.userLevels) {
-      url = this.props.userLevels[0].bonus
+    let url;
+    if (this.props.levelsWithProgress?.length) {


Just curious, when will this.props.levelsWithProgress be undefined/null?

When level/progress data is being loaded.

jamescodeorg · 2021-08-09T17:58:26Z

dashboard/test/controllers/api_controller_test.rb

+    assert_response :forbidden
+  end
+
+  # test "" do


Ooops, will remove this.

jamescodeorg · 2021-08-09T18:01:15Z

apps/src/code-studio/progressRedux.js

@@ -262,6 +265,13 @@ export default function reducer(state = initialState, action) {
    };
  }

+  if (action.type === SET_RELOAD_TEACHER_PANEL_PROGRESS) {


Could you elaborate a bit on why we need this value in redux? Is there a scenario where we need to re-render but redux-managed state doesn't change? (Usually, we just update the underlying data and the appropriate components re-renders, right?)

Right - we're not storing the teacher panel bubble data in redux, it's stored in the TeacherPanel component state, since that's the only component that needs to know about the data. However when a teacher makes a change in the feedback component we need to communicate to the TeacherPanel component that it needs to refresh it's data so that's what's happening here.

Ah, ok, I think I understand now. Do we use this pattern anywhere else? (This feels potentially simpler than putting the data in redux but it also feels a bit non-standard.) Let's chat tomorrow.

jamescodeorg

I think I understand this change now. Let's chat tomorrow and see if we can wrap this up! :-)

(Sorry for the off-hours review, I'm working slightly strange hours this week.)

jamescodeorg · 2021-08-10T03:15:35Z

apps/src/code-studio/components/progress/teacherPanel/TeacherPanel.jsx

+      viewAs === ViewType.Teacher && sectionData && sectionData.level_examples;
+
+    const displayLockInfo =
+      hasSections && unitHasLockableLessons && viewAs === ViewType.Teacher;


(very minor) Maybe move the viewAs condition to be the first in the list to be more consistent with the other two checks above? That should make it clear that all of these only apply to ViewType.Teacher.

We probably don't want to fix this now, but I'm also wondering how sectionData relates to hasSections (i.e. can one be derived from the other)?

I created this ticket https://codedotorg.atlassian.net/browse/LP-1987, there are still many aspects of the teacher panel logic and how data is included that are confusing, hopefully I'll get a chance to address some of this there.

jamescodeorg · 2021-08-10T03:31:11Z

apps/src/code-studio/progressRedux.js

@@ -262,6 +265,13 @@ export default function reducer(state = initialState, action) {
    };
  }

+  if (action.type === SET_RELOAD_TEACHER_PANEL_PROGRESS) {


Ah, ok, I think I understand now. Do we use this pattern anywhere else? (This feels potentially simpler than putting the data in redux but it also feels a bit non-standard.) Let's chat tomorrow.

…ep-working-progress

maureensturgeon · 2021-08-11T19:45:31Z

apps/eslint/index.js

@@ -18,7 +18,8 @@ module.exports = {
            (item.type === 'ExportDefaultDeclaration' &&
              item.declaration.type === 'ClassDeclaration') ||
            (item.type === 'ExportNamedDeclaration' &&
-              item.declaration?.type === 'ClassDeclaration')
+              item.declaration &&


I was getting some linting errors related to this.. just going to clean that up so it doesn't happen to anyone else.

jamescodeorg

Yay!

maureensturgeon and others added 8 commits July 28, 2021 13:45

on teacher feedback review state change, reload progress bubbles on page

590d685

clean up teacher panel loading state, fix header bubbles updating

eb33d5e

Tweaks to new teacher panel API

b85929b

refactor teacher panel components start fixing tests

74922af

Add tests for new teacher_panel_progress API.

83de6c6

update teacher panel tests

2a7f292

fix issues remaining in apps tests

6ba7077

Fix unit test failures

3ea8b94

maureensturgeon force-pushed the maureen/LP-1898-keep-working-progress branch from 2318c4c to 3ea8b94 Compare July 28, 2021 20:51

maureensturgeon marked this pull request as ready for review July 28, 2021 22:58

maureensturgeon commented Jul 30, 2021

View reviewed changes

maureensturgeon requested a review from jamescodeorg August 3, 2021 21:35

jamescodeorg reviewed Aug 9, 2021

View reviewed changes

jamescodeorg reviewed Aug 10, 2021

View reviewed changes

maureensturgeon added 3 commits August 11, 2021 09:09

teacher panel with redux

0e268c4

wire up feedback to teacher panel refetch

179388d

Merge remote-tracking branch 'origin/staging' into maureen/LP-1898-ke…

bea5ab5

…ep-working-progress

maureensturgeon commented Aug 11, 2021

View reviewed changes

jamescodeorg approved these changes Aug 11, 2021

View reviewed changes

fix teacher panel test

3b9d945

maureensturgeon merged commit 2ec7ff7 into staging Aug 11, 2021

maureensturgeon deleted the maureen/LP-1898-keep-working-progress branch August 11, 2021 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On teacher feedback review state change, reload progress bubbles on page #41708

On teacher feedback review state change, reload progress bubbles on page #41708

maureensturgeon commented Jul 22, 2021 •

edited

maureensturgeon Jul 30, 2021

jamescodeorg Aug 9, 2021

jamescodeorg Aug 10, 2021

maureensturgeon Jul 30, 2021

jamescodeorg Aug 10, 2021

jamescodeorg left a comment

jamescodeorg Aug 9, 2021

maureensturgeon Aug 9, 2021

jamescodeorg Aug 9, 2021

maureensturgeon Aug 9, 2021

jamescodeorg Aug 9, 2021

jamescodeorg Aug 9, 2021

maureensturgeon Aug 9, 2021

jamescodeorg Aug 10, 2021

jamescodeorg left a comment

jamescodeorg Aug 10, 2021

maureensturgeon Aug 11, 2021

jamescodeorg Aug 10, 2021

maureensturgeon Aug 11, 2021

jamescodeorg left a comment

On teacher feedback review state change, reload progress bubbles on page #41708

On teacher feedback review state change, reload progress bubbles on page #41708

Conversation

maureensturgeon commented Jul 22, 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

Choose a reason for hiding this comment

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

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

jamescodeorg left a comment

Choose a reason for hiding this comment

maureensturgeon commented Jul 22, 2021 •

edited