Fix: removed video from playlist is still associated with the playlist in history

[Shadorc]

Pull Request Type

• Bugfix
• Feature Implementation
• Documentation
• Other

Related issue

closes #5870

Description

This one was a lot of digging.
When a video is added to the history db, there are three fields that are set to indicate if it was part of a playlist when watched. These fields are lastViewedPlaylistId, lastViewedPlaylistType and lastViewedPlaylistItemId, they are used to display the playlist view after clicking on the video from the history.
When a video is deleted from a playlist, these fields are not updated.

Testing

Scenario 1: the video is part of only one playlist
Reproduce steps in #5870

Scenario 1bis: the video is part of only one playlist
Reproduce previous steps but change tab within 5 seconds after removing the video from the playlist.

Scenario 2: the video is part of more than one playlist

1. Add a video to two playlists ('a' and 'b')
2. Watch the video from playlist 'a'
3. Delete video from playlist 'b'
4. Check that when the video is clicked from history, it still show playlist 'a'

Scenario 3: the video was added in a playlist through the quick bookmark button

1. Add a video to a playlist using the quick bookmark button
2. Watch the video from the playlist
3. Delete the video from playlist through the quick bookmark button
4. Check that when the video is clicked from the history, it does not show any playlist

Additional context

Naming was hard on this one. I take every suggestions that could make this clearer.
Also, I could have used array toBeDeletedPlaylistItemIds to populate with videosId and playlistId but I didn't want to introduce any breaking changes regarding this array so I went with another one.

[Shadorc]

I was thinking about maybe reducing store dispatch calls by pre-checking the playlist instead of checking it in unsetLastViewedPlaylist, but the 5-second deletion delay creates a race condition risk in the following scenario:

• User watches a video from playlist A
• We remove it from playlist B
• We pre-check: "Is B the last viewed playlist?" No, so the dispatch call is skipped
• During those 5 seconds, the user starts watching the same video from playlist B
• The timer expires and deletes the video from B

Result: The bug still occurs.

This is why I let the database query make the final decision, even though it's a bit less optimized regarding IPC calls

[efb4f5ff-1298-471a-8973-3d47447115dc]

Scenario 1 passed but when i tried a slightly modified variant, the video was still associated with the playlist

VirtualBoxVM_UFeyr19WGJ.mp4

[Shadorc]

Interesting, thanks for testing!
The bug was occurring when removing a video from a playlist and then going to another tab under 5 seconds.
In this scenario, removeToBeDeletedVideosSometimes is called from onBeforeRouteLeave. This is where it gets a bit weird. removeToBeDeletedVideosSometimes is async, but it is not awaited. This means that the page is changing while the method runs. The first dispatch gets the correct value of playlistId, but the second gets undefined.
The correct fix is probably to await before leaving the route. It may not be supported, or maybe it hides another issue, but when doing so I get a missing param 'id' error message.
I went for the easy fix: storing the playlist ID before it gets changed.

[efb4f5ff-1298-471a-8973-3d47447115dc]

1,1b and 2 pass

1c doesnt see clip below

VirtualBoxVM_QoOXPrYMCq.mp4

[Shadorc]

Thanks for finding this!

Indeed, removing a video from a playlist can be done from multiple entry points, and the bookmark icon is one of them.

Instead of trying to find every path that leads to a video being removed from a playlist, I think it would be better to directly plug the fix at the lowest level, which is in the playlist store.
I have a concern: this approach would mean that the removeVideo, removeVideos, and removeAllVideos methods from the playlist store would also make a call to the history store. I don't know if that's considered bad practice or if it is fine.

Another solution that comes to my mind would be to have another method that handles both calls to the history and playlist stores, and this would be called instead of directly calling the playlist store.

I can also still search for all paths that lead to videos being removed from playlists, but that doesn't feel future-proof.

What do you think?

[absidue]

Dispatching action calls between stores is fine, the most important thing is to make sure you don't end up creating a loop (store 1 calling into store 2 and store 2 calling into store 1 in response etc).

[Shadorc]

I have added scenario 1c to the testing section. It has been fixed in a3b5541 (sorry for the yarn.lock, it shouldn't happen again).

To be consistent with the fix, I have also taken into account all the other methods that could impact a playlist or a video in a playlist (a478bda). It is not strictly necessary because if a playlist is deleted from removeAllPlaylists, removePlaylist, or removePlaylists, history won't show the playlist since it already checks if the playlist exists. Concerning removeAllVideos, it is not used.

However, I feel like having an outdated database is worse than adding additional database operations that are not strictly useful. It is handled currently, but it could lead to bugs in the future.

removeAllPlaylists and removePlaylists could be optimized to make one call for all playlists instead of one per playlist, but I first want to check with you what you prefer: reverting the commit or keeping the fix fully implemented (and maybe optimize this a little).