#13168 create composable for exam report data fetching for learn exam report viewer #13203
Conversation
… for LearningExamReportViewer
…reate-composable-for-exam-report-data-fetching-for-LearnExamReportViewer
- Remove examReportViewer module and also make changes to the pluginModule.js - refactor the classesRoutes.js which is previously using vuex state
github-actions
bot
added
APP: Learn
|
Thanks @a-s-t-e-y-a! We will review in the upcoming weeks. I wanted to note that review waiting times are still longer. |
|
@LianaHarris360 @MisRob Thank you so much for this. I apologize again for tagging everyone, as I didn't know it would take time. I'm ready to make any necessary changes that you may ask for. |
|
No problem @a-s-t-e-y-a, you didn't do anything bad. Part of my role is a bit of moderation when needed so future collaboration can be happy for everyone, that's all. |
nucleogenesis
left a comment
nucleogenesis
left a comment
There was a problem hiding this comment.
Thank you for your patience @a-s-t-e-y-a and thank you for your efforts on this. On review, I think that this moves in the right direction, but there are a few questions I have that will better help us understand some parts of your approach.
| setError(null); | ||
| } catch (err) { | ||
| router.replace({ | ||
| name: ClassesPageNames.CLASS_ASSIGNMENTS, | ||
| params: { classId }, | ||
| }); |
There was a problem hiding this comment.
Wondering if this should also be setting the error when we catch one here.
There was a problem hiding this comment.
Yeah, you’re right to point this out. I’ve fixed it and set the error correctly here.
| // Global state to replace Vuex | ||
| const globalState = reactive({ | ||
| pageLoading: false, | ||
| pageName: '', | ||
| error: null, | ||
| }); |
There was a problem hiding this comment.
I'm not seeing how/where this is used outside of this module.
The related Vuex state global to the scope of the core Vuex module, but this is only local to this new module and it isn't exported so it seems non-operational.
I could only find one place where we pass the Vuex store into the useContentLink composable function so if you were including this reactive object with the idea of updating Vuex state, then I think you'll need to take the Store as a param and use that interact w/ Vuex stores.
There was a problem hiding this comment.
Separately, if the idea here is to make sure that this state is global across multiple components which import & call it, we have leaned more into using provide/inject to create a more compact scope for the module where you can provide it at the topmost part of the component hierarchy as a sort of "root" of the module's state, which provides it for the descendants to inject.
You can see here an example of this.
To be clear - I'm not really suggesting that you do this but sharing for information.
As far as I can tell, there is no need to provide/inject anything here or to have any state outside of the scope of the module so long as it's being imported and referenced in the single LearnExamReportViewer component.
There was a problem hiding this comment.
I did that and implemented a fix in the code. Having this reactive object with the concept of updating the Vuex state isn't required here, so I chose to leave it in the composable with shared refs. I've resolved that.
| }); | ||
|
|
||
| // Keys for provide/inject | ||
| const EXAM_REPORT_KEY = Symbol('examReport'); |
There was a problem hiding this comment.
I've never seen Symbol used before - does it provide any additional value here than just setting EXAM_REPORT_KEY='examReport'?
There was a problem hiding this comment.
Symbol ensures an exclusive key (examReport) that cannot be copied or replaced by other code. This prevents naming collisions, enhancing code security and maintainability.
But if this is not viable I can do this to the simple by making only "examReport"
| class="container" | ||
| > | ||
| <KCircularLoader v-if="loading" /> | ||
| <KCircularLoader v-if="isLoading" /> |
There was a problem hiding this comment.
This is related to my note about the Vuex <> reactive state in the composable. loading here is referring to a core Vuex module's state which can be affected by any part of the app (ie, during routing, parent components' loading things, etc).
With that in mind, however, I think we're trying to move away from using that Vuex core/global state for loading, so your management of that state in the useExamReport module is on the right track there.
So - the main thing to have in mind here (particularly for any QA) is that we test on slower connections to ensure that this page wasn't expecting other behaviors to set that global loading state to true.
To be clear, no changes requested here at the moment, but we'll be sure to keep this in mind during testing.
There was a problem hiding this comment.
okay I'll keep this in mind
Build Artifacts
|
…reate-composable-for-exam-report-data-fetching-for-LearnExamReportViewer
|
Hi @a-s-t-e-y-a, are you still planning to resolve feedback? |
|
Closing due to inactivity. @a-s-t-e-y-a if you wish to return to this task, let us know. |
4 participants
Summary
After adding the composable in the exam view report, the same functionality is achieved.
Kolibri.mp4
Manual Test Cases Run:
<pre-commit run --all-files>:<yarn run test>:…
References
kolibri/plugins/learn/assets/src/composables/useExamReport.jskolibri/plugins/learn/assets/src/views/LearnExamReportViewer.vue…
Reviewer Guidance
@LianaHarris360 Kindly review my PR. If there are any improvements, I'm happy to hear them and willing to make changes.
This is my first contribution to Kolibri, so I apologize if I've done anything incorrectly.
…