Moved core notifications handling into NotificationsRoot component #12644

iamshobhraj · 2024-09-05T19:19:40Z

Description

Moved core notifications handling from actions.js file to NotificationRoot component that were earlier handled through core vuex state. Removed vuex mutations in kolibri_app.js and its spec file.

issue addressed

Addresses #12626
Removes use of vuex in favour of composables.

Changelog

Description :- Moved core notifications handling into NotificationsRoot component
Addresses :- Move core notifications handling into NotificationsRoot component #12626
Components :- NotificationRoot Component
Breaking :- no

github-actions · 2024-09-05T22:38:18Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0.dev0_git.20241008191942.pex
Windows Installer (EXE)	kolibri-0.18.0.dev0+git.20241008191942-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0.dev0+git.20241008191942-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0.dev0+git.20241008191942.dmg
Android Package (APK)	kolibri-0.18.0.dev0+git.20241008191942-0.1.4-debug.apk
TAR file	kolibri-0.18.0.dev0+git.20241008191942.tar.gz
WHL file	kolibri-0.18.0.dev0+git.20241008191942-py2.py3-none-any.whl

nucleogenesis

Thank you for your PR! I've added a note about a property that was removed that I'm not sure was intentional.

Additionally, it appears you've included two empty config files .bas_hprofile and .bash_profile.

The notifications code looks good to me, but I'll see about testing this locally to be sure there are no regressions.

nucleogenesis · 2024-09-05T22:35:59Z

kolibri/core/assets/src/views/NotificationsRoot.vue

        notificationModalShown: true,
      };
    },
    computed: {
-      ...mapState({
-        error: state => state.core.error,


I'm not seeing why error was removed here, was this intentional?

mapState was removed to completely decouple the component from vuex store, but i did not think of error handling. Should i handle the error locally in getnotification method or keep the mapState for error property?

@nucleogenesis i removed the empty bashrc files. As for the error handling, it is being handled locally in the methods. If you want me to handle error with mapState, should i do that?

My apologies for missing this last week @iamshobhraj

The mapState function is assigning this.error to the value of state.core.error so that this.error is reactive whenever state.core.error changes.

Elsewhere in the component, this.error is expected to exist so we should keep mapState imported and keep this particular line mapping to the error.

To be a bit more clear, basically you just need to remove the line notifications: ... but not the error: ... line.

iamshobhraj · 2024-09-15T13:03:50Z

@nucleogenesis Could you please tell me whats causing this error. I removed the empty bash files, fixed the import errors but I am still getting an error with this api endpoint while testing.

/home/secant/opensource/new/kolibri/kolibri/core/assets/src/api-resource.js:512
      return this._url ? this._url : this.resource.collectionUrl();
                                                   ^

TypeError: this.resource.collectionUrl is not a function
    at Collection.get (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/api-resource.js:512:52)
    at /home/secant/opensource/new/kolibri/kolibri/core/assets/src/api-resource.js:347:27
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v18.20.4
/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:232
  return data.map(function (notification) {
              ^

TypeError: Cannot read properties of undefined (reading 'map')
    at _notificationListState (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:232:15)
    at _callee$ (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:168:37)
    at tryCatch (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:1062)
    at Generator.<anonymous> (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:3008)
    at Generator.next (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:1699)
    at asyncGeneratorStep (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:22:70)
    at _next (/home/secant/opensource/new/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:23:163)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

nucleogenesis · 2024-09-16T22:37:34Z

@iamshobhraj - I'm not sure exactly what the cause would be. Seems that notifications here is ending up undefined/null.

kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue

Lines 155 to 156 in 9011352

    
                       const notifications = await PingbackNotificationResource.fetchCollection(); 
        
                       this.notifications = _notificationListState(notifications);

Do you see any errors in your dev tools' "Network" tab for the request for the notifications? Maybe there is a permissions or other issue at hand

I'm not seeing anything along these lines, but I can't help but wonder if something about the change from using then() to using async/await in handling the PingbackNotificationResource.fetchCollection() call.

rtibbles · 2024-09-24T00:21:15Z

@iamshobhraj - I just tested your PR locally, and did not see the error you are seeing. It tends to indicate that something has gone wrong in the backend, because the URL has not been properly registered. My first suggestion would be to make sure that your Python dependencies in your virtual environment are up to date https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#install-python-dependencies

If that's still not working, I'd recommend running make clean in the project directory, and rerunning the dev server.

What I see when I run your code is that it seems to be working as intended, with the only exception being the issue that @nucleogenesis previously pointed out.

iamshobhraj · 2024-09-27T20:50:45Z

@rtibbles @nucleogenesis I'm sorry for late response. I made the changes requested. But i am still getting this error while testing locally after setting up the project again on my machine. notifications is getting set to undefined and PingbackNotificationResource.fetchCollection(); isnt returning any data.

home/secant/kolibri/kolibri/core/assets/src/api-resource.js:512
      return this._url ? this._url : this.resource.collectionUrl();
                                                   ^

TypeError: this.resource.collectionUrl is not a function
    at Collection.get (/home/secant/kolibri/kolibri/core/assets/src/api-resource.js:512:52)
    at /home/secant/kolibri/kolibri/core/assets/src/api-resource.js:347:27
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v18.19.0
/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:239
  return data.map(function (notification) {
              ^

TypeError: Cannot read properties of undefined (reading 'map')
    at _notificationListState (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:239:15)
    at _callee$ (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:175:37)
    at tryCatch (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:1062)
    at Generator.<anonymous> (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:3008)
    at Generator.next (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:21:1699)
    at asyncGeneratorStep (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:22:70)
    at _next (/home/secant/kolibri/kolibri/core/assets/src/views/NotificationsRoot.vue:23:163)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

rtibbles · 2024-09-27T20:53:05Z

Did you try the steps I suggested here?

My first suggestion would be to make sure that your Python dependencies in your virtual environment are up to date https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#install-python-dependencies

If that's still not working, I'd recommend running make clean in the project directory, and rerunning the dev server.

If the URL doesn't exist, something is going wrong with the Python backend (but the error is being silently caught by Django).

iamshobhraj · 2024-09-27T20:57:30Z

Did you try the steps I suggested here?

My first suggestion would be to make sure that your Python dependencies in your virtual environment are up to date https://kolibri-dev.readthedocs.io/en/develop/getting_started.html#install-python-dependencies

If that's still not working, I'd recommend running make clean in the project directory, and rerunning the dev server.

If the URL doesn't exist, something is going wrong with the Python backend (but the error is being silently caught by Django).

yes, i followed it step by step again on my.

rtibbles · 2024-09-27T21:00:45Z

In the browser console if you enter:

console.log(Object.keys(kolibriCoreAppGlobal.urls))

What do you get as the output (it may be very long)?

iamshobhraj · 2024-09-27T21:18:15Z

console.log(Object.keys(kolibriCoreAppGlobal.urls))

output

rtibbles · 2024-09-27T21:24:09Z

Hrm, so I am seeing the "kolibri:core:pingbacknotification-list", key in the url object, so it is being defined, and passed from the backend, so that can't be the issue.

I will test again locally on my machine, as I still don't quite understand how this is happening and only specifically on your device, seemingly.

iamshobhraj · 2024-09-27T22:39:05Z

@rtibbles i found the issue. notifciations was set to data property which is initially empty. So thats why data.map is thowing an error. I fixed it by checking if notifications is falsy or empty before attempting to map over it. I am really sorry for any inconvenience this may have caused you.

nucleogenesis · 2024-09-30T17:03:02Z

@iamshobhraj thank you for your continued work on this!

I investigated the failing tests and seems like the tests just need to be updated according to the kinds of changes you made.

This commit passes them locally for me: 9f5de38

As for the linting, you'll need to look at the linting issues in the PR actions output, or you can run yarn lint-frontend:format to see the report locally.

nucleogenesis · 2024-10-08T15:31:15Z

Hey @iamshobhraj - would you be able to update your PR? I think that with the linting and tests fixed, this should be ready to merge.

This is work that is currently blocking some upcoming work, so if you don't have time to come back to this someone on our team will likely push some commits here and merge it.

Thank you again for your efforts and work on this!

iamshobhraj · 2024-10-08T15:39:30Z

@nucleogenesis sorry for the delay. I have exams in my university so I was away. I'll make sure to add the changes tonight. Sorry for the inconvenience.

nucleogenesis · 2024-10-08T17:30:47Z

@iamshobhraj no worries at all! If you need time for your exams or study today or can't get to it for any other reason, I can get the PR ready to merge no problem. Thanks again for everything you've done on this! <3

…n its component file

iamshobhraj · 2024-10-08T19:21:46Z

@nucleogenesis i made the changes to test file as per changes made in component file. also fixed the linting error.

nucleogenesis

I re-read the original issue to be sure this covers everything and you checked all of the boxes there! Wonderful work @iamshobhraj thank you!

iamshobhraj added 3 commits September 5, 2024 05:17

issue learningequality#12626

25af6e8

Merge branch 'learningequality:develop' into v1

1b18715

removed vuex mutations

c0ea565

github-actions bot added the DEV: frontend label Sep 5, 2024

[pre-commit.ci lite] apply automatic fixes

9011352

nucleogenesis requested changes Sep 5, 2024

View reviewed changes

removed empty bashrc files

939aab9

iamshobhraj requested a review from nucleogenesis September 8, 2024 09:27

iamshobhraj added 3 commits September 11, 2024 01:25

Merge branch 'learningequality:develop' into v1

8f5b998

Merge branch 'learningequality:develop' into v1

d143e87

fixed import errors

07c14d4

[pre-commit.ci lite] apply automatic fixes

d519363

rtibbles assigned nucleogenesis Sep 24, 2024

iamshobhraj added 5 commits September 27, 2024 19:49

Merge branch 'learningequality:develop' into v1

541f969

fixed error handling

1b474dc

fixed errors

37960ba

fixed errors

37fdf08

fixed linting errors

46f491e

pre-commit-ci-lite bot and others added 2 commits September 27, 2024 21:27

[pre-commit.ci lite] apply automatic fixes

3b7a568

fixed notifications data undefining issue

6ce4a0b

[pre-commit.ci lite] apply automatic fixes

9b0b03a

iamshobhraj added 3 commits October 8, 2024 23:40

Merge branch 'learningequality:develop' into v1

4170a21

updated notificationsRoot.spec.js test file as per the changes made i…

d9e03fb

…n its component file

updated notificationsRoot.spec.js test file as per the changes made i…

094e8e7

…n its component file

nucleogenesis approved these changes Oct 9, 2024

View reviewed changes

nucleogenesis merged commit 8a0029f into learningequality:develop Oct 9, 2024
34 checks passed

nucleogenesis mentioned this pull request Oct 9, 2024

Move core notifications handling into NotificationsRoot component #12626

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved core notifications handling into NotificationsRoot component #12644

Moved core notifications handling into NotificationsRoot component #12644

iamshobhraj commented Sep 5, 2024

github-actions bot commented Sep 5, 2024 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Sep 5, 2024

iamshobhraj Sep 6, 2024

iamshobhraj Sep 7, 2024

nucleogenesis Sep 16, 2024

iamshobhraj commented Sep 15, 2024 •

edited

Loading

nucleogenesis commented Sep 16, 2024

rtibbles commented Sep 24, 2024

iamshobhraj commented Sep 27, 2024 •

edited

Loading

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024 •

edited

Loading

nucleogenesis commented Sep 30, 2024

nucleogenesis commented Oct 8, 2024

iamshobhraj commented Oct 8, 2024

nucleogenesis commented Oct 8, 2024

iamshobhraj commented Oct 8, 2024

nucleogenesis left a comment

Moved core notifications handling into NotificationsRoot component #12644

Moved core notifications handling into NotificationsRoot component #12644

Conversation

iamshobhraj commented Sep 5, 2024

Description

issue addressed

Changelog

github-actions bot commented Sep 5, 2024 • edited Loading

Build Artifacts

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Sep 5, 2024

Choose a reason for hiding this comment

iamshobhraj Sep 6, 2024

Choose a reason for hiding this comment

iamshobhraj Sep 7, 2024

Choose a reason for hiding this comment

nucleogenesis Sep 16, 2024

Choose a reason for hiding this comment

iamshobhraj commented Sep 15, 2024 • edited Loading

nucleogenesis commented Sep 16, 2024

rtibbles commented Sep 24, 2024

iamshobhraj commented Sep 27, 2024 • edited Loading

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024

rtibbles commented Sep 27, 2024

iamshobhraj commented Sep 27, 2024 • edited Loading

nucleogenesis commented Sep 30, 2024

nucleogenesis commented Oct 8, 2024

iamshobhraj commented Oct 8, 2024

nucleogenesis commented Oct 8, 2024

iamshobhraj commented Oct 8, 2024

nucleogenesis left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2024 •

edited

Loading

iamshobhraj commented Sep 15, 2024 •

edited

Loading

iamshobhraj commented Sep 27, 2024 •

edited

Loading

iamshobhraj commented Sep 27, 2024 •

edited

Loading