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

Fix: importing subscriptions with terminated channels #3816

Merged

Conversation

ChunkyProgrammer
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer commented Jul 26, 2023

Fix: importing subscriptions with terminated channels

Pull Request Type

  • Bugfix

Related issue

closes #3735

Description

Import subscriptions when terminated channels found, etc.

Testing

See: #3735 (comment)

Change file extension .txt to .db

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.18.0

Additional context

@efb4f5ff-1298-471a-8973-3d47447115dc I could not find the freetube-subscriptions.json that you mentioned

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 26, 2023 19:27
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 26, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

@ChunkyProgrammer ah sorry about that. See #3735 (comment) to get json file

@PikachuEXE
Copy link
Collaborator

When I import that freetube-subscriptions.json from #3735 (comment)
FT does nothing beside this message (no channel imported)
image

@efb4f5ff-1298-471a-8973-3d47447115dc

That matches with my foundlings when looking at latest dev branch in #3735 (comment)

But idk why it does nothing

@PikachuEXE
Copy link
Collaborator

Tested [SubscriptionsError.csv](https://github.com/FreeTubeApp/FreeTube/files/12018412/SubscriptionsError.csv)
image
image

PikachuEXE
PikachuEXE previously approved these changes Jul 27, 2023
@ChunkyProgrammer
Copy link
Member Author

freetube-subscriptions.json should work now

@PikachuEXE
Copy link
Collaborator

Can someone explain what's channel.service_id === 0?

@ChunkyProgrammer
Copy link
Member Author

Can someone explain what's channel.service_id === 0?

Newpipe has different services (bandcamp, youtube, etc.). 0 is the id for youtube but it can probably be removed so we check the url instead

@PikachuEXE
Copy link
Collaborator

Yes please
Or at least add a code comment about it (very confusing when reading the code)

@PikachuEXE
Copy link
Collaborator

freetube-subscriptions.json now works (but no error, expected?)

@ChunkyProgrammer
Copy link
Member Author

freetube-subscriptions.json now works (but no error, expected?)

I believe so (that file was only erroring because there was no service id in it so all subscriptions were filtered out)

only check url, not service_id
@@ -1056,7 +1055,7 @@ export default defineComponent({
const channel = await getLocalChannel(channelId)

if (channel.alert) {
return undefined
return []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that returning an empty array is inline with the other parts of this file, however I think we should change that in all places, because we don't check for the empty array anywhere.

e.g. on this line we check if the author property of the return value is undefined, so when we return an empty array we are checking if [].author is undefined, as that property never exists, the check succeeds. Checking an array for a property that never exists seems wrong to me, either we should be checking for an empty array, or change the return values to be something other than an empty array, so that the checks make more sense.

if (typeof channelInfo.author !== 'undefined') {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR? (Since I have no idea what do you mean

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also more difficult to review with more changes, this is just to fix importing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it in a separate pull request sounds good to me. The current solution works, so technically the bug is fixed, it's just that the way it's done in the whole file, including this pull request, is bad and makes the code confusing. I suspect it's leftover from a previous refactoring.

Hopefully chunky understands what I mean, otherwise I can do it myself.

Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as per discussion above.

Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • profiles.txt imported successfully without any errors

  • subscriptions.csv imported successfully but i did get an error in the console but i think that was expected
    Capture

  • SubscriptionsError.csv imported successfully but i did get an error in the console but i think that was expected
    Capture

  • SubscriptionsNoError.csv imported successfully without any errors

  • Test.Subscriptions.txt imported successfully without any errors

  • https://jumpshare.com/v/hrxb5KFMfmqOda4egFFN imported successfully without any errors

If those errors are indeed expected, i'll approve.

@ChunkyProgrammer
Copy link
Member Author

@efb4f5ff-1298-471a-8973-3d47447115dc those errors are expected

@FreeTubeBot FreeTubeBot merged commit 2296e3a into FreeTubeApp:development Aug 1, 2023
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 1, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Aug 5, 2023
* development: (33 commits)
  Miscellaneous CSS cleanup (FreeTubeApp#3847)
  Fix empty channels showing up as errored with RSS (FreeTubeApp#3824)
  Fix author for album playlists on the playlist page (FreeTubeApp#3838)
  Update Snap Source Host Location (FreeTubeApp#3844)
  * Show error message in popular tab when instance does not support it (FreeTubeApp#3841)
  Use video durations from the watch history for RSS (FreeTubeApp#3839)
  ! Fix unnecessary error message display in toast when paused before video started playing on load (FreeTubeApp#3835)
  Use emit and props instead of $parent (FreeTubeApp#3834)
  Add custom toast event bus for Vue 3 compatiblity (FreeTubeApp#3833)
  Fix handling of DeArrow titles (FreeTubeApp#3825)
  * Update top nav bar icon to remove focus state style (FreeTubeApp#3792)
  Update ft-input for top navbar search input to behave more like Youtube one (FreeTubeApp#3793)
  Translated using Weblate (Hungarian)
  Fix: importing subscriptions with terminated channels (FreeTubeApp#3816)
  Fix outdated subscription cache clearing code when "Remove All Subscriptions / Profiles" performed (FreeTubeApp#3817)
  Translated using Weblate (Croatian)
  Bump eslint-plugin-import from 2.27.5 to 2.28.0 (FreeTubeApp#3827)
  Bump eslint from 8.45.0 to 8.46.0 (FreeTubeApp#3829)
  Bump eslint-plugin-unicorn from 48.0.0 to 48.0.1 (FreeTubeApp#3828)
  Bump lefthook from 1.4.6 to 1.4.7 (FreeTubeApp#3830)
  ...

# Conflicts:
#	src/renderer/components/ft-list-video/ft-list-video.js
#	src/renderer/components/playlist-info/playlist-info.js
#	src/renderer/components/playlist-info/playlist-info.vue
#	src/renderer/components/watch-video-info/watch-video-info.js
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Aug 5, 2023
* feature/playlist-2023-05: (31 commits)
  Miscellaneous CSS cleanup (FreeTubeApp#3847)
  Fix empty channels showing up as errored with RSS (FreeTubeApp#3824)
  Fix author for album playlists on the playlist page (FreeTubeApp#3838)
  Update Snap Source Host Location (FreeTubeApp#3844)
  * Show error message in popular tab when instance does not support it (FreeTubeApp#3841)
  Use video durations from the watch history for RSS (FreeTubeApp#3839)
  ! Fix unnecessary error message display in toast when paused before video started playing on load (FreeTubeApp#3835)
  Use emit and props instead of $parent (FreeTubeApp#3834)
  Add custom toast event bus for Vue 3 compatiblity (FreeTubeApp#3833)
  Fix handling of DeArrow titles (FreeTubeApp#3825)
  * Update top nav bar icon to remove focus state style (FreeTubeApp#3792)
  Update ft-input for top navbar search input to behave more like Youtube one (FreeTubeApp#3793)
  Translated using Weblate (Hungarian)
  Fix: importing subscriptions with terminated channels (FreeTubeApp#3816)
  Fix outdated subscription cache clearing code when "Remove All Subscriptions / Profiles" performed (FreeTubeApp#3817)
  Translated using Weblate (Croatian)
  Bump eslint-plugin-import from 2.27.5 to 2.28.0 (FreeTubeApp#3827)
  Bump eslint from 8.45.0 to 8.46.0 (FreeTubeApp#3829)
  Bump eslint-plugin-unicorn from 48.0.0 to 48.0.1 (FreeTubeApp#3828)
  Bump lefthook from 1.4.6 to 1.4.7 (FreeTubeApp#3830)
  ...
@ChunkyProgrammer ChunkyProgrammer deleted the fix-subscriptions branch August 15, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot import any data from Freetube, Newpipe, Youtube, or Invidious.
5 participants