-
Notifications
You must be signed in to change notification settings - Fork 867
Introduce copy class functionality #13517
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
Introduce copy class functionality #13517
Conversation
Build Artifacts
|
78aa70f to
b6ceced
Compare
nucleogenesis
left a comment
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've added a few notes here and found a few bugs in testing. Please focus on these first before the following note.
One additional issue I ran into is that the KDropdown is behaving oddly. When I open the menu and click copy, rename, delete - they all work.
But when using the keyboard, only the delete option does anything - the other two just close the dropdown but don't trigger their expected behaviour.
I did some console log debugging and I can tell you that the code you have in your handleOptionSelection handler is running properly where a console.log inside of your if(selection.value === Modals.COPY_CLASS) blocks will run based on which was clicked... just for some reason it seems the value is not being sent.
For example - I have this: console.log(selection, row) at the top of the function and when I click or use the keyboard, the same value comes through the console.
I'm not sure what to do here so once the comments I left are resolved, try to give this a look and we can chat about it on Slack as needed.
| /> | ||
|
|
||
| <ClassRenameModal | ||
| v-if="openRenameModal" |
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.
The cancel button on the modal doesn't work for some reason. I see that closeModal dispatches an action classManagement/displayModal.
It might be worth considering just tracking which modal is visible locally within the component here rather than leaning on the Vuex action function.
| classCoachesIds.value = classDetails.value.coaches | ||
| .map(coach => coach.id) | ||
| .filter(id => id !== undefined); |
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.
Did you find coaches that had an undefined ID value?
| <template #default> | ||
| <div> | ||
| <KTextbox | ||
| :value="copyOfClass$({ class: classDetails.name })" |
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.
The value here must be something reactive like a ref because this way the title of the class is always going to be "Copy of Class" even when I type something else into the textbox here.
I suggest something like const copiedClassName = ref(null); - then where you open the modal, set it's value to the copyOfClass$ string w/ the class-to-be-copied's name.
Then, when the modal closes, be sure to set the copiedClassName.value = null again so that the value isn't present the next time you open the side panel.
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.
Noting that copying the same class twice will replicate the error I got for this
kolibri/plugins/coach/assets/src/views/quizzes/QuizSummaryPage/ManageExamModals.vue
Outdated
Show resolved
Hide resolved
LianaHarris360
left a comment
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.
If I'm not mistaken, I believe that all of the coaches in the facility should be listed in the side panel, with only the coaches assigned to the original class being pre-selected.
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
nucleogenesis
left a comment
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.
The notes I added here are blocking - but one non-blocking thought is that we should clarify with Tomiwa about if the coaches should be pre-selected upon opening the panel or if they should be fully deselected.
| classCoachesIds.value = classDetails.value.coaches.map(coach => coach.id); | ||
| classCoaches.value = facilityUsers.value |
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.
Why does classCoaches.value get assigned by way of the facilityUsers var but the classCoachesIds is set using classDetails?
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.
If classCoachesIds is representing the value of "what coaches are selected" and the classCoaches is representing the list of coach objects passed as options to SelectableList it seems like they should be derived from the same source.
When I tested this, the selectable list was empty for all classes despite them having coaches in them.
I made one change: classCoaches.value = classDeatils.value.coaches.map() (no need for the filter since we're using class details, but I kept the map() call. This fixed the missing coaches 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.
Why does
classCoaches.valueget assigned by way of thefacilityUsersvar but theclassCoachesIdsis set usingclassDetails?
I was trying to list all coaches in the facility and only pre-select the coaches that belong to the selected class as @LianaHarris360 had suggested.
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.
😅 that makes sense - thanks for clarifying
| isClassNameInvalid() { | ||
| return this.tableRows.some(row => row[0] === this.copiedClassName); | ||
| }, |
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 is flashing the message to the user before closing the side panel when the copying is successful.
This condition will be evaluated before the completion of handleSubmitClassCopy because once this line is evaluated:
this.$store.commit('classManagement/SET_STATE', { classes: updatedClasses });
That's where the tableRows is updated. You could maybe try closing the side panel before calling that commit in the function to mitigate this issue.
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.
@AllanOXDi I'm still seeing this flash.
Perhaps a flag like this.submitting or this.loading could be added and set to true until the very end of the handleSubmitClassCopy function where you set it back.
Then in the isClassNameInvalid add a && !this.submitting to the end of it.
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 added a note about console errors I'm seeing and identified the cause of why I see no coaches on my end.
I have 1000 user and like 20 of them are coaches. When you're getting facilityUsers from the useUserManagement composable you're only getting the first page of users which, in my case, results in there being no coaches shown.
I suggest rather than calling useUserManagement() that you could basically just use FacilityUser.fetchCollection sort of how it is in that composable but with the params like kind: UserKinds.COACH so that the query only fetches the kinds of users we need here.
Something that isn't in scope for this PR - but we really should consider - is making the handleSelection handler here leverage useUserManagement's coupling to the route query params so that the user's text input searches by way of the API (w/ the kind=UserKinds.COACH in the params).
I don't think a facility will be likely to have 100+ coaches but we should certainly be prepared for it. (cc @marcellamaki @radinamatic)
|
|
||
| <ClassRenameModal | ||
| v-show="openRenameModal" | ||
| :classname="classDetails.name" |
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.
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 don't think a facility will be likely to have 100+ coaches but we should certainly be prepared for it
Yes, it seems unlikely, but also worth being prepared for/refactoring for in follow up.
route query params so that the user's text input searches by way of the API (w/ the kind=UserKinds.COACH in the params)
So a backend match for keyword (well, name filtering) search?
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.
Yeah that's kind what I was thinking but coming back to it I'm less confident that'd work so well as we'd need to re-call the composable each time the search changed.
nucleogenesis
left a comment
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.
Added a couple comments - the blocker is the flashing error message. Let me know if my suggestion is unclear there and we can sort it out together.
| const fetchFacilityUsers = async () => { | ||
| const resp = await FacilityUserResource.fetchCollection({ | ||
| getParams: pickBy({ |
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.
pickBy here isn't doing anything so we can remove it
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.
For more context - pickBy is used to conditionally take key-value pairs from an object based on a callback function's boolean return value. Since your using all of the values in the object anyway, no need to pickBy anything.
For example:
const obj = { a: 1, b: 2, c: 3 };
const odds = pickBy(obj, val => val % 2 !== 0); // { a: 1, c: 3 }
const evens = pickBy(obj, val => val % 2 === 0); // { b: 2 }| isClassNameInvalid() { | ||
| return this.tableRows.some(row => row[0] === this.copiedClassName); | ||
| }, |
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.
@AllanOXDi I'm still seeing this flash.
Perhaps a flag like this.submitting or this.loading could be added and set to true until the very end of the handleSubmitClassCopy function where you set it back.
Then in the isClassNameInvalid add a && !this.submitting to the end of it.
AlexVelezLl
left a comment
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.
Hi @AllanOXDi! Just took a look, and noted couple of things :)
| :label="option.label" | ||
| /> | ||
| > | ||
| <template v-if="displayUserRole"> |
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.
(Blocking) We shouldn't be hard-coding this in the SelectableList component. The SelectableList component is meant to be a generic component that renders and accessible selectable searchable list. It shouldn't expose a displayUserRole prop and hardcoded coach role tag here.
What we can do instead to have more flexibility of how we render the options is to expose an optionLabel slot that takes the option as parameter. Such as:
<slot name="optionLabel" :option="option"></slot>and then in the parent component we can compute the label we have with the userTypeDisplay and all the styles we need,without making this general-purpose component specific to a given context.
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
| @cancel="closeModal" | ||
| @success="handleRenameSuccess()" | ||
| /> | ||
| <SidePanelModal |
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 we should move this to be its own component. Its bloating too much this ManageClassPage component.
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
| } | ||
| }; | ||
| fetchFacilityUsers(); |
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.
Just a heads up that we are loading these coaches on every ManageClassPage render, even if we dont want to copy the class. Moving the class copy to its own component will prevent this.
kolibri/plugins/facility/assets/src/views/ManageClassPage/index.vue
Outdated
Show resolved
Hide resolved
| type="text" | ||
| :label="classTitleLabel$()" | ||
| :autofocus="true" | ||
| :maxlength="120" |
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 should be 100 to match the API (I tried making a max-length class name and got an API error)
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.
Ooops! Taken care of
2de9496 to
0773cb5
Compare
nucleogenesis
left a comment
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.
Tested out and everything works great. Thanks for extracting that side panel out - it really makes the index file much cleaner (thanks for suggesting @AlexVelezLl !).

Summary
This pull request introduces the ability to make copy of classes, along with improvements to class management actions, on the ManageClassPage.
References
Closes #13443
Reviewer guidance