learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
801 stars 659 forks source link

New icon is shown in channel when a resource was deleted. #6730

Closed cpauya closed 10 months ago

cpauya commented 4 years ago

Observed behavior

When a resource is deleted, there is a New icon shown in the channel name.

This "New" icon is removed when the Clear completed link is clicked.

kolibri-v0 13 2-b1--deleting a resource puts a New icon in a channel

Expected behaviour

Show a different icon when a resource is deleted in a channel. The New is more for new (or updated) contents.

User-facing consequences

The New icon is confusing with the delete action.

Steps to reproduce

  1. Install Kolibri v0.13.2-beta1 in Ubuntu 16.04.6 VM
  2. Use Firefox then import a channel
  3. Delete a resource within the topic tree of the channel. Follow this Gherkin scenario for an example.
  4. After the resource is deleted, DO NOT click the Clear or Clear completed buttons.
  5. Click the X button to leave the task manager immersive modal.
  6. Notice the "New" icon in the channel name.

Fix Acceptance Criteria (added by @nucleogenesis )

Context

indirectlylit commented 4 years ago

thanks, agreed

riddhiavlani commented 3 years ago

When a resource is deleted from a channel by a user, is there a need to indicate it on the channel? I think the 'new' tag that appears as shown above shouldn't appear at all

indirectlylit commented 3 years ago

that sounds fine to me and the easiest change

an alternative would be a new label like 'Updated' which could apply to all updates, not just additions

riddhiavlani commented 3 years ago

Yes, 'Updated' label for any sort of edit to the channel resources would work better: Frame 1 (4).png

rtibbles commented 1 year ago

I think it probably is - you will need to follow the steps for replication to confirm your fix, so make sure to do it before you change any code to confirm!

tripathics commented 1 year ago

@nucleogenesis I've set up the codebase and started working on this issue. Kindly assign it to me

MisRob commented 1 year ago

Hi @tripathics, thank you for your interest, I'm assigning you. Please note that this is a time-sensitive issue since we'd like to have it in the next release, so it's possible that at some point one of the team members will need to resolve it.

tripathics commented 1 year ago

Hi @tripathics, thank you for your interest, I'm assigning you. Please note that this is a time-sensitive issue since we'd like to have it in the next release, so it's possible that at some point one of the team members will need to resolve it.

@MisRob what should the new icon say for removed resources? I was thinking of Resources removed but that seemed very long. I also thought of having no icon at all. Kindly let me know

MisRob commented 1 year ago

@tripathics Please do not add new strings, this issue is planned for the next release, and due to some internal process in regard to translation, we can't add new strings at this point. According to one of the comments above (https://github.com/learningequality/kolibri/issues/6730#issuecomment-885430580), it should say "Updated". We already have that string in our codebase, however, it seems to be in different plugins. There'd be more ways to approach it and I'd like to check on it with the team before we proceed here, so please wait for the reply.

MisRob commented 1 year ago

@tripathics So we decided it'd be best to simply add the new "Update" string and include this issue in some of the later releases. However, it may take us a couple of weeks to merge your pull request as we can't be merging work with new strings into develop at this point. If you don't mind, feel free to start working on it.

If you haven't done so already, please have a look at our developer documentation. For this particular issue, it may be helpful to read through the Internationalization section. This issue is to be resolved on the develop branch so please open a pull request into develop. If you have any problems running Kolibri locally or have questions, you can use the comments section on this page. As part of your pull request, you're welcome to add yourself to the list of contributors in AUTHORS.md file.

tripathics commented 1 year ago

Thank you for the feedback @MisRob .

MisRob commented 1 year ago

@tripathics You're welcome, let me know if you needed anything

tripathics commented 1 year ago

@MisRob I have my exams going this week. So I will be submitting the PR after that. If anyone is interested, please let them contribute. Else I'll be available after this week

MisRob commented 1 year ago

Sure, thank you for letting us know. No problem at all, as mentioned this would be waiting for merge anyways. Good luck with your exams.

nikkuAg commented 1 year ago

Is this issue still open? I would like to contribute on it.

rtibbles commented 1 year ago

Sure - I'll assign you.

thesujai commented 1 year ago

@rtibbles Is this issue still reproducible? When i delete a resource i dont see a Back to channels link

rtibbles commented 1 year ago

You're right the 'back to channels' link is no longer there - but pressing the X in the top left of the immersive page layout should have the same effect.

I have updated the issue description to reflect this!

thesujai commented 1 year ago

@rtibbles I am seeing no such icons: Screencast from 18-09-23 10:55:11 AM IST.webm

I dont even see new when downloading new channel

marcellamaki commented 1 year ago

Hi @thesujai - It does look from your screencast that the new label is missing. As a next step, you could look for where that new label is referenced in the Vue component, and check the logic, as it would need to be updated to resolve this bug. I'm guessing there is probably one or more v-if statements and some computed values that determine the conditions when this should be displayed. We do want the new label to appear when downloading a new channel, but not when deleting a resource from the channel.

Let us know if you have and more questions! Hopefully this is able to unblock you here :)

nikkuAg commented 1 year ago

When I run the command yarn run devserver the server is not working. It is showing uncaught referenceerror: kolibricoreappglobal is not defined, it was working before but suddenly it has started showing this error and the webpage is stuck on loading icon.

MisRob commented 1 year ago

@nikkuAg Replied in https://github.com/learningequality/kolibri/pull/11191

thesujai commented 1 year ago

Hey @marcellamaki, I tried debugging it and what is observed was the new label was dependend on prop showNewLabel. and whether that prop is true or not depends on this condition https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/views/ManageContentPage/index.vue#L208 The taskIndex is always -1 for all the channel(new or not), so the condition always becomes false and the new label are never shown no matter the channel is new or not

marcellamaki commented 1 year ago

Hi @thesujai - it seems like you were able to do some great debugging! That does seem like an incorrect condition for the showNewLabel prop. Do you have any specific questions I can help with?

Otherwise, you might look into kolibri/plugins/device/assets/src/modules/manageContent/index.js, where installedChannelsWithResources is defined. There have been some changes to our tasks in the last months, so it's possible that there need to be some adjustments in how we are creating the task index. Seeing what is returned with each channel, and the various tasks and task statuses, could be a good next step.

thesujai commented 1 year ago

@marcellamaki ok looking into it

thesujai commented 1 year ago

What could be the logic to show updateBadge, i see for showing newBadge we check whether the task type is not of type DISKCONTENTEXPORT or DISKEXPORT or DELETECHANNEL. So shall i consider it to be TaskTypes.UPDATECHANNEL? Is there any documentaion to this?

MisRob commented 12 months ago

What could be the logic to show updateBadge, i see for showing newBadge we check whether the task type is not of type DISKCONTENTEXPORT or DISKEXPORT or DELETECHANNEL. So shall i consider it to be TaskTypes.UPDATECHANNEL? Is there any documentaion to this?

Hi @thesujai, I am not sure if you still need some guidance as there's already a pull request. Let us know if there's something needed.

MisRob commented 11 months ago

@thesujai @rtibbles @bjester Can this issue be closed now after https://github.com/learningequality/kolibri/pull/11484 or is there some more work?

rtibbles commented 11 months ago

I think with #11484 merged this issue should now be replicable should it still exist (which I imagine it probably does). So this can still be worked on!

thesujai commented 11 months ago

@rtibbles how would suggest for this logic to be implemented? Currently new badge is shown based on task list, should update also follow similar pattern?

rtibbles commented 11 months ago

The issue seems to be specifically about deleting a resource from a channel explicitly - so I think maybe we just shouldn't apply the 'new' label if the task for the channel was a resource deletion task!

It seems we are already excluding the DELETECHANNEL task here: https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/device/assets/src/modules/manageContent/index.js#L53

So, I think the first thing to do is to confirm that this behaviour is extant, and if so, why this task type exclusion isn't preventing it.

If it is not extant any more, then we can close this issue.

thesujai commented 10 months ago

@rtibbles Yes i can confirm that the new is not showing when a resource from the channel is deleted from channel the new badge is not shown. But this is only one case. The problem begins when someone imports more to channel, the new badge is shown. Now if without clearing the task if someone deletes a resource from the channel, the new badge will be shown but not because the resource was deleted but because there is a task which is there for updation on the channel.