Quiz Creation - remove UUID, change section reference approach

nucleogenesis commented 2 months ago

Overview

In useQuizCreation the Exam object is the source of truth - so when things are updated, there is a private/local _quiz object which is updated, then all reactive state derives from that object.

When I started writing this, I envisioned that each section would probably need it's own unique ID so I used uuid for it.

@rtibbles pointed out that the use of uuid is unnecessary here and suggested a separate approach to tracking which section is active.

Overall, moving away from using a section_id should improve performance as we will not be cycling a list filtering for an ID, but rather using the specific index to get what we need from an array.

How to approach this

As the feature has matured, the use of section_id for things like template element :key values, conditionals (ie, if( section.section_id == currentSectionId.value) being used to guard behavior in a map callback), and for routing - to name a few.

useQuizCreation overall

I think that starting by implementing the new architecture first so that it can work in-line with the current architecture would be an ideal approach.

Add a property activeSectionIndex which can track the index of the currently selected section
Update the activeSection computed property so that it uses activeSectionIndex to get the correct section
Ensure that as the selected section changes, the activeSectionIndex is updated properly
Ensure that the activeSectionIndex is updated and accounted for while handling rearranging of the sections or deleting sections

Once you have that working you should then be able to basically grep for cases of section_id being called and solve it one block at a time.

For example, take this bit of code from the end of updateSection -- this is a case where we are mapping over the allSections object and only updating the currently selected section.

      // Update matching QuizSections with the updates object
      question_sources: get(allSections).map(section => {
        if (section.section_id === section_id) {
          return { ...section, ...updates };
        }
        return section;
      }),

An alternative approach, using the new index approach, can be to copy the sections, update the section by the index, then assign that to the question_sources property:

const updatedSection = { ...get(activeSection), ...updates }
const question_sources = get(exam).question_sources;
question_sources[get(activeSectionIndex)] = updatedSection;
/* then where we're saving the data we can just pass the question_sources object */
      // Update matching QuizSections with the updates object
      question_sources,

Routing

Currently the section_id is used to indicate by the router which section you're actively using. So to use the index instead, we will need to be sure to use $router.replace to change the route in place without pushing to the history. This ought to ensure a seamless experience using browser back/forward functionality.

Template code

Generally I think that places where things use the section.section_id to make a unique ID for something, you can probably just use the index and some unique to that situation text (ie, section-header-${index}).

Acceptance criteria

[ ] No mention of section_id anywhere - be sure to check specs, comments in exam.models, exam serializer, etc
[ ] Remove uuid altogether from all related code (Coach)

nucleogenesis commented 3 weeks ago

Just checking back on this, there are these places that I find section_id in develop today - makes me wonder how many bugs there might be lurking because of this 🤔

kolibri/plugins/coach/assets/test/useQuizCreation.spec.js|99 col 33| expect(get(activeSection).section_id).toEqual(get(allSections)[0].section_id);
kolibri/plugins/coach/assets/test/useQuizCreation.spec.js|139 col 40| get(allSections).find(s => s.section_id === addedSection.section_id),
kolibri/plugins/coach/assets/test/useQuizCreation.spec.js|148 col 40| get(allSections).find(s => s.section_id === addedSection.section_id).section_title,
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|98 col 25| :key="section.section_id"
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|104 col 31| activeSection.section_id === section.section_id ? activeSectionStyles : borderStyle
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|325 col 52| allSections.value.map(section => section.section_id),
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|326 col 57| sectionOrderList.value.map(section => section.section_id),
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|442 col 71| const reorderedId = this.allSections[this.activeSectionIndex].section_id;
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|444 col 30| section => section.section_id === reorderedId,
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|461 col 80| const sectionOrderIds = this.sectionOrderList.map(section => section.section_id);
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|463 col 46| return sectionOrderIds.indexOf(a.section_id) - sectionOrderIds.indexOf(b.section_id);
kolibri/plugins/coach/assets/src/views/plan/CreateExamPage/SectionEditor.vue|496 col 18| s => s.section_id === section.section_id,
kolibri/plugins/coach/assets/src/composables/quizCreationSpecs.js|107 col 35| * @property {string}             section_id                 A unique ID for the section - this is
kolibri/plugins/coach/assets/src/composables/quizCreationSpecs.js|118 col 3| section_id: {
kolibri/plugins/coach/assets/src/composables/useQuizCreation.js|258 col 30| delete sectionToSave.section_id;

nucleogenesis commented 3 weeks ago

I'll flesh this out a bit more, but just noting that @rtibbles noted that one key place where we do use section_id is to generate a client-side-only unique key for use in the v-for.

So perhaps my intention here to nix it altogether is too heavy-handed.

And I suppose that if the section_id has a use, isn't being persisted to the backend, and isn't hurting anybody we can leave it and all of it's current uses be where they are well suited.

So, with that said, this issue can be resolved after a more thorough/thoughtful review of the files listed in my above comment if there are no changes deemed necessary

learningequality / kolibri