-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix single quiz selection flow #12274
Fix single quiz selection flow #12274
Conversation
c3c0d15
to
18ba3f5
Compare
Build Artifacts
|
Hi @rtibbles,
Server.error.mp4
no.escape.mp4
validation.mp4
delete.stuff.mp4Logs: logs.zip |
18ba3f5
to
6902b14
Compare
Thanks @pcenov! |
I think I have addressed all issues bar 3 - which I think @nucleogenesis is tackling elsewhere. I think this also addresses #12246. It also updates the resource selection flow to be about adding new questions specifically, with each 'update resources' allowing you to choose how many questions you are selecting and choosing resources to fill those questions. I implemented this after discussion with @marcellamaki and @nucleogenesis about the counterintuitiveness of selecting resources first and only then deciding the number of questions. Interest in thoughts and feedback, although I think a lot of the strings could definitely do with being reworded. |
9f54b98
to
90c2133
Compare
Hi @rtibbles, I've had a look at the latest changes here and:
select.quiz.mp4
add.section.mp4 |
Irritating, I had both those errors in development, but thought I had addressed them - sorry for the wild goose chase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual testing notes
Resource selection # questions selected, resources previously selected
2024-06-13.09-06-46.mp4
Number of questions changes not save-able unless you change your resource selection (and the selectAll is visible at the end when it shouldn't be)
2024-06-13.09-09-00.mp4
Adding resources adds a number of questions in excess of the defined question_count
on save, but does not impact the value in the form input
2024-06-13.09-10-22.mp4
90c2133
to
8b263bd
Compare
@pcenov I replicated and think I have fixed both of the issues you experienced. @nucleogenesis I updated copy to make it clear that all of the issues you raised are now intended behaviour (we have also talked about this extensively in person and on slack). |
Hi @rtibbles - I confirm that both issues are fixed now. I'll just mention here some other issues which I noticed while regression testing but maybe it's better to have everything merged and then to test everything again:
No.snackbar.that.the.changes.have.been.saved.mp4
replace.questions.mp4
modal.mp4 |
Yeah, I can fix 3 here, as that text has been added within this PR, but the other issues are probably best dealt with elsewhere! We are planning on holding off merging this PR until Monday, so that we can test out the original workflow for managing questions by adding and removing resources vs the newer workflow of just incrementally adding questions from resources that I have implemented here. Glad to know that it is generally working though! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one blocking error and some comments - my review isn't complete yet but I have to come back to finish later on.
@@ -483,7 +619,12 @@ | |||
fetchMoreResources, | |||
resetWorkingResourcePool, | |||
contentPresentInWorkingResourcePool, | |||
//contentList, | |||
questionCount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contentPresentInWorkingResourcePool
(named on the line above) is failing for me when I go through to the QA Channel -> Exercises
I have a KA Perseus topic which has no children so when the card for that KA Perseus topic is passed to contentPresentInWRP
it has no children, which chokes up the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might just work well to filter out empty topics to begin with - which I thought we'd been doing 🤔
class="content-checkbox" | ||
:label="content.title" | ||
:showLabel="false" | ||
:checked="contentIsChecked(content)" | ||
:indeterminate="contentIsIndeterminate(content)" | ||
@change="handleCheckboxChange(content, $event)" | ||
/> | ||
<KRadioButton | ||
v-else-if="contentHasCheckbox(content) && showRadioButtons" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking, minor bug:
When I select the quiz and hit the confirm button, the KRadioButton disappears briefly before redirecting. It is very pronounced on "Fast 3G" throttling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe "just use this on Fast 3G for a bit and fix what you see" could be a solid theme for a cohack, some things I noticed before I couldn't handle it anymore:
Note, I have my cache disabled as well, so all typically cached API calls & resources would likely be much faster, but this is worth polishing
- Click "Add section" -- the section tab is added... then nothing for a few seconds... then the slideout appears w/ the section settings
- Click "Add questions" -- basically the same, nothing happens - the page doesn't start spinning... it just sits there until the new content is displayed
- Basically a lot of transitions are met with a blank stare and sudden change rather than a spinner or some other indication of "something is happening just wait a sec"
Again -- not blocking, just noticed during testing here and can put into follow-up today
const EXERCISES = [ | ||
{ | ||
id: 'A', | ||
title: 'title_x', | ||
assessmentmetadata: { assessment_item_ids: ['A1', 'A2', 'A3'] }, | ||
}, | ||
{ | ||
id: 'B', | ||
title: 'title_y', | ||
assessmentmetadata: { assessment_item_ids: ['B1', 'B2', 'B3', 'B4', 'B5', 'B6', 'B7', 'B8'] }, | ||
}, | ||
{ | ||
id: 'C', | ||
title: 'title_z', | ||
assessmentmetadata: { | ||
assessment_item_ids: ['C1', 'C2', 'C3', 'C4', 'C5', 'C6', 'C7', 'C8', 'C9'], | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much prettier <3
export function exerciseToQuestionArray(exercise) { | ||
return exercise.assessmentmetadata.assessment_item_ids.map((question_id, i) => { | ||
return { | ||
exercise_id: exercise.id, | ||
question_id, | ||
counter_in_exercise: i + 1, | ||
// In the V3 schema, the title is user editable, and no longer | ||
// simply the title of the exercise the question came from. | ||
// We set it to blank to indicate that no user generated title | ||
// has been created. | ||
title: '', | ||
item: `${exercise.id}:${question_id}`, | ||
}; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this
9b23123
to
0dade88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking issue:
- Select a practice quiz with only a few questions
- Select those questions and then click to replace them
- You shouldn't have enough replacements, now if you click "Add resources" on that model, you open the Resource Selection
The issue is that the resource side panel opens to the Practice Quiz resource which I've already added instead of the channels list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save followed by immediate Save & Close isn't working here:
2024-06-14.16-29-54.mp4
I think both of the things you observed here should now be fixed @nucleogenesis - I have also tweaked the warning modal shown when there are not enough replacements to better represent the current state in this PR. |
I'm struggling a bit around wrapping my brain around all the changes here... 😅 Some re-wording suggestions:
replacements-counter.mp4 |
Update exam utils spec to properly assert conversions being tested against V3.
Update QuestionListPreview for just read only usage. Update display of questions.
Just return to root quiz page on save.
…time. Tweak question replacement.
… across multiple sections.
Update v3 conversion logic to constrain section length.
Make string parameterized in case of changes.
Ensure we don't show any topics without children, even if the unfiltered num assessments says otherwise.
Update strings for replacements modal to reflect updated workflow.
672237d
to
9783504
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good - let's merge and the issues noted in the bug bash can be replicated and made into issues today
Summary
Before:
After:
References
Fixes #12247
Reviewer guidance
Ensure you have practice quizzes imported.
Choose the 'select a quiz' option from the create quiz dropdown.
Ensure you can select a quiz and then create a quiz from there.
Ensure that this has not induced any regressions in the main quiz creation experience.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)