Synchronise user selections with Quiz Creation State #11783

ozer550 · 2024-01-25T08:00:14Z

Summary

Implementation of a _working_resource_pool ref in useQuizCreation to persist user-selected exercises across route navigations.
Initialization of the working resource pool on opening the side panel and clearing it upon closing, ensuring accurate tracking of changes.
Real-time updating of the working resource pool as users interact with checkboxes for exercise selection and deselection.
Integration of a confirmation modal when closing the side panel with unsaved changes.
Incorporation of select all and intermediate state of checkbox.

References

Solves the first part of #11734

Reviewer guidance

For side Panel:

- Go to Coach->Plan->Quiz.
- Create new Quiz.
- Add some exercises to the quiz and try to close the side panel.
- Confirmational now works as expected.
  (ConfirmCancellationModal.vue)

For working_resource_pool logic

- Add some excercises to Quiz.
- Go to section tab of quiz again.
- Click on edit Section.
- Change resources.
- When you visit already added excercises they would already be checked. 
(working_resource_pool utility in useQuizCreation and wiring in other components)

…

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

Automated test coverage is satisfactory
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

Co-authored-by AllanOXDi <[email protected]>

Add working resource pool logic

github-actions · 2024-01-25T08:30:29Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b13.dev0_git.491.gc5154348.pex
Windows Installer (EXE)	kolibri-0.16.0b13.dev0+git.491.gc5154348-unsigned.exe
Debian Package	kolibri_0.16.0b13.dev0+git.491.gc5154348-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b13.dev0+git.491.gc5154348-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b13.dev0+git.491.gc5154348-0.1.0-debug.apk
TAR file	kolibri-0.16.0b13.dev0+git.491.gc5154348.tar.gz
WHL file	kolibri-0.16.0b13.dev0+git.491.gc5154348-py2.py3-none-any.whl

nucleogenesis

In manual testing things work as expected!

Noting that the related issue's "select questions" business will be moved to a separate issue -- so validating that this works properly involved checking the Vue devtools and it all worked as expected, more or less.

Overall, feedback is largely formatting / clean-up releated and suggestions for JSDocs.

The one thing that I'm not sure for the cause of is that we cannot save the quiz, getting Must be a valid UUID errors for every item in the resource pool -- I think that this may just be a matter of using import { v4 } from 'uuid'; in useQuizCreation when we generate the section_id.

Note -- that with @ozer550 out I'll likely take on some of these fixes and force push to the branch so that we can try to get this ready to merge before next iteration.

nucleogenesis · 2024-01-25T18:59:52Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionSidePanel.vue

+    </SidePanelModal>
+    <ConfirmCancellationModal
+      v-if="showConfirmationModal"
+      :closePanelRoute="closePanelRoute"


Need to handle the @closeModal event here (could just be an inline function like @closeModal=() => showConfirmationModal = false

nucleogenesis · 2024-01-25T19:02:17Z

kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/ContentCardList.vue

@@ -65,6 +65,7 @@
 <script>

  import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
+  import { injectQuizCreation } from '../../../composables/useQuizCreation';


Looks like all of the changes on this file can be removed as the injected methods aren't used in this component

nucleogenesis · 2024-01-25T19:06:09Z

kolibri/plugins/coach/assets/src/composables/useQuizCreation.js

+   * Adds resources to _working_resource_pool
+   */
+  function addToWorkingResourcePool(resources = []) {
+    set(_working_resource_pool, [...get(_working_resource_pool), ...resources]);


I think we should wrap the array here with lodash's uniqWith and isEqual alike to:

uniqWith([...get(_working_resource_pool), ...resources], isEqual) -- this will use the lodash isEqual function to check equality of the objects in the list (see the conveniently relevant example in the lodash docs).

nucleogenesis · 2024-01-25T19:07:11Z

kolibri/plugins/coach/assets/src/composables/useQuizCreation.js

@@ -445,12 +465,40 @@ export default function useQuizCreation(DEBUG = false) {
    }
  });

+  /**
+   * Adds resources to _working_resource_pool


Let's get the JSDoc types up-to-date here w/ @param and @affects entries - the type for the resources param ought to be QuizExercise[]

nucleogenesis · 2024-01-25T19:11:08Z

kolibri/plugins/coach/assets/src/composables/useQuizCreation.js

+  }
+
+  /**
+   * Check if the content is present in working_resource_pool


JSDoc suggestion:
@param {QuizExercise} content - Check if the content is present...
@returns Boolean

nucleogenesis · 2024-01-25T20:51:30Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

    computed: {
      isTopicIdSet() {
        return this.$route.params.topic_id;
      },
+      isSelectAllChecked() {
+        //if all the reosurces in contentList are present in working_resource_pool


Formatting: reosurces -> resources

nucleogenesis · 2024-01-25T20:52:12Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

+      // contentIsInLesson() {
+      //   return ({ id }) => Boolean(this.channels);
+      // },


This can be removed

nucleogenesis · 2024-01-25T20:53:16Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

+      // addableContent() {
+      //   // Content in the topic that can be added if 'Select All' is clicked
+      //   const list = this.contentList.value ? this.contentList.value : this.bookmarksList;
+      //   return list.filter(
+      //     content => !this.contentIsDirectoryKind(content) && !this.contentIsInLesson(content)
+      //   );
+      // },


This can be removed - particularly with the uniqWith/isEqual suggestion elsewhere in this review in place

nucleogenesis · 2024-01-25T20:54:31Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

+      // saveResources() {
+      //   this.saveQuiz();
+      // },


Is this commented out because it is no longer used?

nucleogenesis · 2024-01-25T20:57:34Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ResourceSelection.vue

+      handleSearchTerm(searchTerm) {
+        const query = {
+          last_id: this.$route.query.last_id || this.$route.params.topicId,
+        };
+        const lastPage = this.$route.query.last;
+        if (lastPage) {
+          query.last = lastPage;
+        }
+        this.$router.push({
+          name: LessonsPageNames.SELECTION_SEARCH,
+          params: {
+            searchTerm,
+          },
+          query,
+        });
+      },
+      handleMoreResults() {
+        this.moreResultsState = 'waiting';
+        this.fetchAdditionalSearchResults({
+          searchTerm: this.searchTerm,
+          kind: this.filters.kind,
+          channelId: this.filters.channel,
+          currentResults: this.searchResults.results,
+        })
+          .then(() => {
+            this.moreResultsState = null;
+            this.moreResultsState;
+          })
+          .catch(() => {
+            this.moreResultsState = 'error';
+          });
+      },
+      */


I thought that this had been removed in my last PR -- can we strip out the search business here as it'll be addressed in an upcoming issue.

rtibbles

I am not seeing anything blocking here, per se - but I think it is worth reflecting on the granularity of each set of state.

One of the ongoing issues that we've had with vuex is that there is one granularity of state - global - and this has caused issues and edge cases where state has not been properly reset, updated etc.

I know that you've tried to preempt that by having a specific state reset function, but if we instead encapsulated the state at a finer level of granularity, then we would be able to pre-empty any of these issues, rather than having to add additional code to fix them.

rtibbles · 2024-01-27T00:30:16Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

@@ -558,6 +567,8 @@
        set(this.dragActive, true);
      },
      openSelectResources(section_id) {
+        //initialize workingResourcePool
+        this.initializeWorkingResourcePool();


Is there a reason this couldn't happen within the ResourceSelection component instead?

rtibbles · 2024-01-27T00:30:41Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/CreateQuizSection.vue

@@ -382,6 +385,8 @@
        allSections,
        activeSection,
        inactiveSections,
+        activeResourcePool,


Both of these are added, and neither are used.

rtibbles · 2024-01-27T00:31:36Z

kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/ConfirmCancellationModal.vue

+    setup() {
+      const {
+        //Computed
+        resetWorkingResourcePool,


I think it would be better to make this component emit an event rather than handle all state clearing and route changing.

rtibbles · 2024-01-27T00:35:05Z

kolibri/plugins/coach/assets/src/composables/useQuizCreation.js

@@ -477,8 +548,13 @@ export default function useQuizCreation(DEBUG = false) {
  return {
    // Methods
    saveQuiz,
+    initializeWorkingResourcePool,


This is turning the useQuizCreation composable into a bit of a catch all again - I think all the state and methods here are at a smaller level of granularity than the useQuizCreation composable is handling.

I think with some judicious work, this could all be instantiated entirely within the ResourceSelection component, or in a separate composable that is only used inside the ResourceSelection component.

I think the main issue is handling that the component re-setups each navigation, hence it living in then composable. I think I initially suggested that it live in ResourceSelection but foresaw it being a problem until we fix the coach routing issues

rtibbles · 2024-01-27T00:37:50Z

kolibri/plugins/coach/assets/src/composables/useQuizCreation.js

-    return ExamResource.saveModel({ data: get(_quiz) });
+
+    // Here we update each section's `resource_pool` to only be the IDs of the resources
+    const sectionsWithResourcePoolAsIDs = get(allSections).map(section => {


Couldn't we just always maintain the frontend representation of this the same as the backend, and have a separate representation of the resources that map to these IDs that we populate when we start editing a specific section? (in the proposed separate state management within ResourceSelection that I propose below).

I specced it this way so that we have access to all of the Exercise metadata that we've already fetched at hand so we can do the ancestry calculations and get the assessmentids for question enumeration.

This is related to the issue I made and closed yesterday re: updating the serializer to take the object rather than the IDs - but since that's only a concern in the case that we edit a saved quiz (in which case we'd need to fetch the metadata).

I figured that this way reduces our needing to fetch the metadata wherever it is needed here while keeping things roughly in the shape of the backend data - except on the front end we keep the whole object whereas we just send the IDs.

To be honest I'm not entirely sure this needs to be saved at all except for in the case that we permit editing / duplication of quizzes as the 'questions' property has everything needed to render the quiz. The resource_pool is solely used for enumerating the total possible pool of questions the coach can choose from to add to the quiz.

Coming back to this after the weekend, I think that it makes sense to maintain a separate map of the metadata and to keep the list of IDs in the section's data withing the useQuizCreation module.

This would then be easy to extend to accommodate duplication/editing of a quiz or section as we can then populate the initial map with a single API call for the list of node IDs there.

nucleogenesis

@rtibbles

I've addressed the clean-up comments.

I'm going to add a note re: "refactor working resource pool to live in ResourceSelection.vue, rather than useQuizCreation, once rerouting between folders no longer results in remounting the component" to #11741 and we can hopefully aim for it in a planned patch release.

I'm approving & merging now to be sure issues we plan to assign tomorrow can be started.

AllanOXDi and others added 14 commits January 22, 2024 15:06

add confirm model

a44adac

add helper function realted with working_resource_pool

a4608ac

complete set setters and getters related with workingResourcePool

c696280

Co-authored-by AllanOXDi <[email protected]>

Merge pull request #1 from ozer550/add-working-resource-pool-logic

bc0b264

Add working resource pool logic

section questions get updated

8ebc4ab

select all adds and removes resources from the pool

4ed3c5a

select all adds and removes resources from the pool

90eb803

working pool reset on saving changes

42d85c2

only checked resources are added to the working resource pool

534b431

removed double addition of resource to the working pool

98e88d6

removed unused component

6cfff4d

make ConfirmationalModal work

d29b873

integrate ConfirmationalModal working with closing sidePanel

595d92a

fix small bugs for select all checkboxes

960d29b

github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels Jan 25, 2024

fix linting

7e5dd31

ozer550 requested review from AllanOXDi and nucleogenesis January 25, 2024 08:16

AllanOXDi assigned nucleogenesis Jan 25, 2024

nucleogenesis requested changes Jan 25, 2024

View reviewed changes

nucleogenesis added 7 commits January 25, 2024 14:08

handle @cancel event from ConfirmCancellationModal

775434c

revert unused injection of quiz creation stuff into ContentCardList

5628303

make add/remove/exists? resource selection API consistent; add JSDocs

1e88a7a

minor code simplification

90f1b6f

comments update; remove search-related unused methods

8149baa

remove unused borrowed code

76eb986

remove reference to saveQuiz in ResourceSelection as it is not used

2379859

remove -'s from uuid's generated in quiz creation for consistency

f9e66da

nucleogenesis mentioned this pull request Jan 25, 2024

Validate QuizSectionSerializer.resource_pool against actually-used information #11798

Closed

3 tasks

nucleogenesis requested review from rtibbles and marcellamaki January 26, 2024 00:06

only pass IDs to API in resource_pool

2cbd234

rtibbles assigned rtibbles and unassigned nucleogenesis Jan 26, 2024

rtibbles reviewed Jan 27, 2024

View reviewed changes

nucleogenesis added 3 commits January 29, 2024 11:42

move resource pool init to ResourceSelection

97ca355

remove unused import/exporst

618fe47

emit continue event from cancellation modal, reset in event handler

9f64cc0

nucleogenesis approved these changes Jan 30, 2024

View reviewed changes

nucleogenesis merged commit 171f6bb into learningequality:develop Jan 30, 2024
31 checks passed

This was referenced Jan 30, 2024

Enhanced Quizzes: UI & UX Updates & Tech Debt Tracker #11741

Closed

Quiz Creation - Bookmarks selection #11818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronise user selections with Quiz Creation State #11783

Synchronise user selections with Quiz Creation State #11783

ozer550 commented Jan 25, 2024

github-actions bot commented Jan 25, 2024 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

nucleogenesis Jan 25, 2024

rtibbles left a comment

rtibbles Jan 27, 2024

rtibbles Jan 27, 2024

rtibbles Jan 27, 2024

rtibbles Jan 27, 2024

nucleogenesis Jan 27, 2024

rtibbles Jan 27, 2024

nucleogenesis Jan 27, 2024

nucleogenesis Jan 29, 2024

nucleogenesis left a comment

Synchronise user selections with Quiz Creation State #11783

Synchronise user selections with Quiz Creation State #11783

Conversation

ozer550 commented Jan 25, 2024

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Jan 25, 2024 • edited Loading

nucleogenesis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles 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

nucleogenesis left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2024 •

edited

Loading