Fix Mismatch between coach Reports and generated CSV #12628

AlexVelezLl · 2024-09-02T19:16:44Z

Summary

Fix mismatch between coach Reports and generated CSV.

Fixes:

Now ReportsLessonExerciseLearnerListPage and ReportsLessonResourceLearnerListPage show ungrouped learners if "view by groups" checkbox is toggled.
Fix ReportsLessonExerciseLearnerListPage and ReportsLessonResourceLearnerListPage to show groups names if we download a report without checking "view by groups".
Fix empty "last activity" column in ReportsLessonExerciseLearnerListPage, ReportsGroupReportLessonExerciseLearnerListPage, ReportsGroupReportLessonResourceLearnerListPage, ReportsLessonResourceLearnerListPage.
New column "All Learners" in ReportsQuizListPage and ReportsLessonListPage.
Fix time spent label not displaying correctly in ReportsGroupReportLessonExerciseLearnerListPage, ReportsGroupReportLessonResourceLearnerListPage, ReportsLearnerReportLessonPage, ReportsLessonExerciseLearnerListPage, ReportsLessonLearnerBase, ReportsLessonResourceLearnerListPage.

Things that have already been fixed in the past:

ReportsQuizLearnerListPage already shows the columns total/answered/correct questions.
No issues found in the option to export a quiz report as CSV.
No issues found about Average score value not displaying correctly in groups reports.

References

Closes #6642

Reviewer guidance

…

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

github-actions · 2024-09-02T19:47:13Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.17.0a0.dev0_git.79.gbfd4d541.pex
Windows Installer (EXE)	kolibri-0.17.0a0.dev0+git.79.gbfd4d541-windows-setup-unsigned.exe
Debian Package	kolibri_0.17.0a0.dev0+git.79.gbfd4d541-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.17.0a0.dev0+git.79.gbfd4d541.dmg
Android Package (APK)	kolibri-0.17.0a0.dev0+git.79.gbfd4d541-0.1.4-debug.apk
TAR file	kolibri-0.17.0a0.dev0+git.79.gbfd4d541.tar.gz
WHL file	kolibri-0.17.0a0.dev0+git.79.gbfd4d541-py2.py3-none-any.whl

pcenov · 2024-09-04T10:39:42Z

Thanks @AlexVelezLl - I confirm that the issues are addressed and fixed as specified above.
While regression testing I was also able to identify the following issues (I can create follow-up issues if they are not in scope for this PR):

In Coach > Reports > Lessons > "Lesson 1" > "Resource 1" when I export the report as CSV I see CommonCoachStrings.timeSpentLabel instead of the the Time spent label and also the Last activity value is displayed only for exercises while for any other type of resources such as PDF or video it's not:

For resources other than exercises if I have selected the 'View by groups' checkbox then when I export the report as CSV I don't see the ungrouped learners:

For all resources if I have not selected the 'View by groups' checkbox then when I export the report as CSV I don't see the name of the groups:

cc @radinamatic

AlexVelezLl · 2024-09-04T14:36:13Z

Thank you @pcenov! I have pushed new changes fixing these issues too.

pcenov

Thanks @AlexVelezLl - I confirm that the latest issues are fixed and there are no new issues after the regression testing - good to go!

rtibbles · 2024-09-05T16:25:35Z

kolibri/plugins/coach/assets/src/csv/fields.js

@@ -216,10 +228,10 @@ export function tally() {
  ];
 }

-export function timeSpent(key, label = 'timeSpentLabel') {
+export function timeSpent(key, label = coreStrings.$tr('timeSpentLabel')) {


I'd prefer to still invoke the $tr method in function scope rather than module scope (which it will for a default) - just in case there's a race condition here between i18n setup and execution of this module code.

It's possible that I am remembering a gotcha that I have since fixed, but out of abundance of caution let's keep the invocation in function scope for now.

AlexVelezLl · 2024-09-06T20:37:46Z

Thank you @rtibbles! I have pushed the required changes 👐.

nucleogenesis

I see that @rtibbles feedback has been addressed, calling the i18n in the function scope and gave things a look myself. This all LGTM

nucleogenesis · 2024-09-06T21:47:13Z

kolibri/plugins/coach/assets/src/views/reports/ReportsLessonExerciseLearnerListPage.vue

@@ -271,8 +271,8 @@
          .reduce((entries, groupEntries) => entries.concat(groupEntries), []);

        if (this.ungroupedEntries.length) {
-          data.concat(
-            this.ungroupedEntries.map(entry => {
+          data.push(


I don't recall having seen using arr.push(...someArray) in place of arr.concat(someArray) before but I like it!

Did you make this change as a style change or do you know if this improves performance or something?

Playing with it a little bit I realize that the push version mutates the object itself whereas concat returns a whole new array so was the previous code an issue because of that?

Yeees!! The error why ungrouped learners didnt appear in the report was because the concat method doesnt work in-place, and we werent reassigning the result, but as data was a constant, and we couldnt reassign I preferred to use the push(...array) instead of defining the constant as variable.

AlexVelezLl · 2024-09-09T13:05:46Z

Thank you @nucleogenesis and @pcenov!

github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend DEV: tools Internal tooling for development labels Sep 2, 2024

AlexVelezLl added 2 commits September 2, 2024 14:26

Fix ungrouped entries missing from report

562e13b

Add All learners column to lesson and quiz list report

e36a1a2

AlexVelezLl force-pushed the fix-reports branch from 301abaa to e36a1a2 Compare September 2, 2024 19:27

AlexVelezLl changed the base branch from develop to release-v0.17.x September 2, 2024 19:27

AlexVelezLl added the TODO: needs review Waiting for review label Sep 3, 2024

AlexVelezLl requested review from rtibbles, nucleogenesis and pcenov September 3, 2024 14:10

AlexVelezLl added 4 commits September 4, 2024 09:06

Fix time spent label

b96996a

Fix last activity columns in all reports

1b64ae7

Fix ungrouped learners missing from non exercise resources

7ab9975

Fix groups names missing from lesson exercise and resources report

7a72442

pcenov approved these changes Sep 5, 2024

View reviewed changes

AlexVelezLl requested review from marcellamaki and removed request for rtibbles September 5, 2024 16:18

rtibbles reviewed Sep 5, 2024

View reviewed changes

Add label default value inside function scope

c1d91bb

AlexVelezLl requested a review from rtibbles September 6, 2024 20:37

nucleogenesis approved these changes Sep 6, 2024

View reviewed changes

AlexVelezLl merged commit aa6eaf2 into learningequality:release-v0.17.x Sep 9, 2024
34 checks passed

AlexVelezLl deleted the fix-reports branch September 9, 2024 13:05

AlexVelezLl mentioned this pull request Sep 9, 2024

Mismatch between coach Reports and generated CSV #6642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Mismatch between coach Reports and generated CSV #12628

Fix Mismatch between coach Reports and generated CSV #12628

AlexVelezLl commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024 •

edited

Loading

pcenov commented Sep 4, 2024

AlexVelezLl commented Sep 4, 2024

pcenov left a comment

rtibbles Sep 5, 2024

AlexVelezLl commented Sep 6, 2024

nucleogenesis left a comment

nucleogenesis Sep 6, 2024

nucleogenesis Sep 6, 2024

nucleogenesis Sep 6, 2024

AlexVelezLl Sep 9, 2024

AlexVelezLl commented Sep 9, 2024

Fix Mismatch between coach Reports and generated CSV #12628

Fix Mismatch between coach Reports and generated CSV #12628

Conversation

AlexVelezLl commented Sep 2, 2024 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Sep 2, 2024 • edited Loading

Build Artifacts

pcenov commented Sep 4, 2024

AlexVelezLl commented Sep 4, 2024

pcenov left a comment

Choose a reason for hiding this comment

rtibbles Sep 5, 2024

Choose a reason for hiding this comment

AlexVelezLl commented Sep 6, 2024

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Sep 6, 2024

Choose a reason for hiding this comment

nucleogenesis Sep 6, 2024

Choose a reason for hiding this comment

nucleogenesis Sep 6, 2024

Choose a reason for hiding this comment

AlexVelezLl Sep 9, 2024

Choose a reason for hiding this comment

AlexVelezLl commented Sep 9, 2024

AlexVelezLl commented Sep 2, 2024 •

edited

Loading

github-actions bot commented Sep 2, 2024 •

edited

Loading