Skip to content
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

Device > Channels > Import > The "Task finished" snackbar is never shown #12965

Open
pcenov opened this issue Dec 20, 2024 · 28 comments
Open

Device > Channels > Import > The "Task finished" snackbar is never shown #12965

pcenov opened this issue Dec 20, 2024 · 28 comments
Assignees
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) help wanted Open source contributors welcome P2 - normal Priority: Nice to have

Comments

@pcenov
Copy link
Member

pcenov commented Dec 20, 2024

Observed behavior

This is a follow-up to #12946 - The "Task finished" snackbar is never shown after successfully importing/exporting or deleting a channel. This is valid for both Kolibri 0.17 and 0.18.

Expected behavior

Should be investigated why is this snackbar not being displayed and if possible it should be restored.

Steps to reproduce the issue

  1. Install the latest develop build or Kolibri 0.17.4
  2. Go to Device > Channels > Import and import/export/delete a channel
  3. Observe that only the "Task started" snackbar is displayed

Usage Details

Windows 11, Ubuntu 22 - Chrome

@pcenov
Copy link
Member Author

pcenov commented Dec 20, 2024

@rtibbles

@rtibbles
Copy link
Member

Thanks @pcenov

@rtibbles rtibbles added P2 - normal Priority: Nice to have help wanted Open source contributors welcome APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) labels Dec 20, 2024
@shruti862
Copy link

@rtibbles @pcenov Is this issue open to work upon??

@rtibbles
Copy link
Member

Yes - ensuring that the task finished snackbar properly displays when a task finishes would be what is needed!

@shruti862
Copy link

@rtibbles Could you please assign the issue to me? I would like to give it a try.

@ananyakaligal
Copy link

@rtibbles @pcenov I would also like to give it a try, could you please assign me this?

@LianaHarris360
Copy link
Member

Thank you both for your willingness to help with this! We only need one person to handle it at a time, and since @shruti862 asked first, I'll assign it to them.

@karankoder
Copy link

Hi @LianaHarris360 , I noticed that this issue is still open, and it seems @shruti862 is not actively working on it. Could you please assign this issue to me? I'd like to take it up

@LianaHarris360
Copy link
Member

Hi @karankoder, we'll wait another week since the holidays are coming to an end. If there's no update by then, I'll hand it over to @ananyakaligal if they are still interested, if not I'll assign to you. @shruti862, can you let me know if you will be working on this?

@shruti862
Copy link

@LianaHarris360 Yes, I am working on it; I have already inspected the cause of issue but due to the holidays I thought to share afterwards ,By tonight I will share my work.

@LianaHarris360
Copy link
Member

Thanks for the update and your thoughtfulness in waiting until after the holidays to share @shruti862! It's great that you're making progress, there's no rush to share it by tonight :)

@shruti862
Copy link

@rtibbles @pcenov ,After inspecting the issue I noticed that watcher on computed property (watchedTaskHasFinished ) is not being triggered in taskNotificationMixin.js file due to which createTaskFinishedSnackbar() method is not called leading to no display of finished Snackbar. But I was unable to inspect the cause of watcher not being triggered because both watchedTaskId and watchedTaskHasFinished values are changing on task starting and on ending..Could you please suggest me something which can help me figure out the problem

computed: {
    watchedTaskId() {
      return this.$store.state.manageContent.watchedTaskId;
    },
    watchedTaskHasFinished() {
      // Should switch between null and the taskIds, and can be watched
      // by component to trigger side effects when a task finishes.
      return this.$store.getters['manageContent/taskFinished'](this.watchedTaskId);
    },
  },
  watch: {
    watchedTaskHasFinished(val) {
        if (val && val === this.watchedTaskId) {
          if (this.showSnackbarWhenTaskHasFinished) {
            this.createTaskFinishedSnackbar();
          }
          if (typeof this.onWatchedTaskFinished === 'function') {
            this.onWatchedTaskFinished();
          }
        }
     
    },
  },

@rtibbles
Copy link
Member

rtibbles commented Jan 7, 2025

Hrm, that is tricky - and this watcher is never called at all?

  watch: {
    watchedTaskHasFinished(val) {
        if (val && val === this.watchedTaskId) {
          if (this.showSnackbarWhenTaskHasFinished) {
            this.createTaskFinishedSnackbar();
          }
          if (typeof this.onWatchedTaskFinished === 'function') {
            this.onWatchedTaskFinished();
          }
        }
     
    },
  },

Or is it called only with a falsy value so it doesn't get past the if statement? Or a value that isn't equal to this.watchedTaskId?

@shruti862
Copy link

@rtibbles, According to my finding watchers is never called, But when I tried to call it on initial render by using immediate:true , it does triggered because at that time it is not dependent on change in a computed property.
watchedTaskId computed property is working fine ,I think the issue resides in taskFinished getter which is being called in watchedTaskHasFinished computed property. But again unable to figure it out .

@AlexVelezLl
Copy link
Member

Hey @shruti862! Are we sure that this taskNotificationMixin is being used in that page? I just added a console.log on mounted, but didnt see any outputs in the console. Can you double check if that mixin is used in the manage tasks page?

@shruti862
Copy link

shruti862 commented Jan 10, 2025

Hey @AlexVelezLl After inspecting using vue devtool I found that in manageChannelContentPage, on clicking delete button handleDeleteSubmit method evoke which has this.onTaskSuccess method which displays 'Task started' Snackbar through notifyAndWatchTask(task)

 handleDeleteSubmit({ deleteEverywhere }) {
        this.beforeTask();
        this.startDeleteTask({
          deleteEverywhere,
          channelId: this.channelId,
          channelName: this.channel.name,
          included: this.selections.included,
          excluded: this.selections.excluded,
        }).then(this.onTaskSuccess, this.onTaskFailure);
      },
      handleExportSubmit({ driveId }) {
        this.beforeTask();
        this.startExportTask({
          driveId,
          channelId: this.channelId,
          channelName: this.channel.name,
          included: this.selections.included,
          excluded: this.selections.excluded,
        }).then(this.onTaskSuccess, this.onTaskFailure);
      },
      beforeTask() {
        this.closeModal();
        this.bottomBarDisabled = true;
      },
      onTaskSuccess(task) {
        this.bottomBarDisabled = false;
        this.watchedTaskType = task.type;
        this.notifyAndWatchTask(task);
      },

and it also have this onWatchedTaskFinished() method which is supposed to be used by taskNotificationMixin to display 'Task Finished' Snackbar

/**
       * @public
       * Used by the taskNotificationMixin to handle the completion of the task
       */
      onWatchedTaskFinished() {
        // clear out the nodeCache
        this.nodeCache = {};
        // clear out selections
        this.$store.commit('manageContent/wizard/RESET_NODE_LISTS');
        // refresh the data on this page. if the entire channel ends up
        // being deleted, then redirect to upper page
        this.fetchPageData(this.channelId)
          .then(this.setUpPage)
          .catch(error => {
            // If entire channel is deleted, redirect
            if (error.response.status === 404) {
              this.$router.replace({ name: PageNames.MANAGE_CONTENT_PAGE });
            } else {
              this.$store.dispatch('handleApiError', { error });
            }
          });
      },

@shruti862
Copy link

@AlexVelezLl I found nothing revelant in ManageTaskPage which is responsible for displaying Snackbar

@rtibbles
Copy link
Member

It seems reasonable that there may be an issue in the reactivity of taskFinished getter, based on your observations. My guess would be is that it is not updating when the underlying vuex state updates.

I would look and see if watching either the task itself, or the task's status triggers a watcher? Then we can just check the status in the watcher to see if it has finished and trigger the snackbar based on that?

@shruti862
Copy link

shruti862 commented Jan 11, 2025

Hey @rtibbles ,When I tried to have tasks computed property, it is not updating with finished tasks, it remains a empty array is it due to reactivity issues? In Vuex store taskList is being updated but not reflecting changes in tasks computed property

computed: {
    watchedTaskId() {
      return this.$store.state.manageContent.watchedTaskId;
    },
    tasks() {
      return this.$store.state.manageContent.taskList;
    },
  },

@shruti862
Copy link

Hey @rtibbles , any suggestions on resolving this reactivity issue would be appreciated so I can start working on it. If there are no updates or if someone else is better suited to take it up, feel free to unassign me so others can contribute. Thanks!

@rtibbles
Copy link
Member

So, it seems the way that the taskList vuex state is being updated is not triggering reactivity. What is the vuex mutation that is updating it? Where is it being updated?

@shruti862
Copy link

Hey @rtibbles , in kolibri/plugins/device/assets/src/modules/manageContent/index.js file I found mutation that is updating taskList

function defaultState() {
  return {
    channelList: [],
    channelListLoading: false,
    taskList: [],
    watchedTaskId: null,
  };
}

export default {
  namespaced: true,
  state: defaultState,
  mutations: {
    SET_STATE(state, payload) {
      Object.assign(state, payload);
    },
    RESET_STATE(state) {
      Object.assign(state, defaultState());
    },
    SET_CHANNEL_LIST(state, channelList) {
      state.channelList = [...channelList];
    },
    SET_CHANNEL_LIST_LOADING(state, isLoading) {
      state.channelListLoading = isLoading;
    },
    SET_TASK_LIST(state, taskList) {
      state.taskList = [...taskList];
    },
    SET_WATCHED_TASK_ID(state, taskId) {
      state.watchedTaskId = taskId;
    },

@rtibbles
Copy link
Member

Hrm, that does look like it shouldn't be falling foul of any of the usual reactive gotchas.

Can you try using the mapState helper function from vuex to define the tasks computed property instead of directly accessing the store to see if that makes a difference?

See here for guidance: https://vuex.vuejs.org/guide/state.html#the-mapstate-helper (looking at how it is used elsewhere in Kolibri will also help!)

@shruti862
Copy link

Hey @rtibbles, Thank you for your suggestion. I will try this approach as well and provide you with updates soon.

@shruti862
Copy link

shruti862 commented Jan 21, 2025

Hey @rtibbles, I tried your suggestion
Code changes:

 computed: {
    ...mapState('manageContent', {
      taskList: state => state.taskList,
      watchedTaskId: state => state.watchedTaskId
    }),
  },

Before deleting resources computed properties were:

Image

After deleting resources, only watchedTaskId is getting updated ( as seen under vuex binding in vue-devtools ),taskList still remains empty array:

Image

@MisRob
Copy link
Member

MisRob commented Jan 21, 2025

Hi @shruti862, would you be able to debug a bit deeper around what's the reactivity issue around taskList? Maybe there is a place in code that doesn't update the array in a way that breaks the reactivity chain? https://v2.vuejs.org/v2/guide/reactivity.html#Change-Detection-Caveats

@MisRob
Copy link
Member

MisRob commented Jan 21, 2025

If this doesn't help then I'd recommend you track step by step places executed in code that (1) set and (2) retrieve the task list and see at what point exactly the mismatch begins

@shruti862
Copy link

@MisRob Thank you for your suggestions I will try to debug it deeper and will provide updates soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) help wanted Open source contributors welcome P2 - normal Priority: Nice to have
Projects
None yet
Development

No branches or pull requests

8 participants