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.

Sign up for GitHub

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

Jump to bottom

Improve Importing Subscriptions #2604

Merged
merged 10 commits into from
Oct 9, 2022
Merged

Improve Importing Subscriptions #2604

merged 10 commits into from
Oct 9, 2022

Conversation

ChunkyProgrammer
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer commented Sep 21, 2022 •
edited
Loading

Improve Importing Subscriptions

Pull Request Type
Please select what type of pull request this is:

  • Bugfix

Related issue
closes #1848 (allows importing subscriptions when a subscription is banned)

Description

  • importing a subscription will autodetect what subscription type it is instead of prompting users to select a subscription type
  • deduplicate code for subscribing to a channel for newpipe and youtube (json & csv)
  • remove 'check for legacy subscriptions' as it is 2 years old and I imagine no one is using a 2 year old version of freetube, this old subscription type can still be imported by locating the file.
  • do not fail when an error is found with a channel (channel banned/ deleted) Local API error when importing youtube subscription #1848 (additional modifications can be made in a future PR to not send youtube requests to prevent rate limiting when importing. Down side: no channel thumbnail)
  • exporting a csv will respect " and ,
  • importing a csv will properly parse names that have " and ,

Testing (for code that is not small enough to be easily understandable)
Has this pull request been tested?
Imported and exported the following:

  • NewPipe (JSON)
  • YouTube (JSON)
  • YouTube (CSV)

Desktop (please complete the following information):

  • OS: Windows
  • OS Version: 11
  • FreeTube version: 0.17.1

Additional context
subscriptions.csv

@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 21, 2022
@PrestonN PrestonN enabled auto-merge (squash) September 21, 2022 17:53
absidue
absidue previously approved these changes Sep 23, 2022
View reviewed changes
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 23, 2022
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 23, 2022
@ChunkyProgrammer ChunkyProgrammer removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 23, 2022
@ChunkyProgrammer ChunkyProgrammer marked this pull request as draft September 23, 2022 17:04
auto-merge was automatically disabled September 23, 2022 17:04

Pull request was converted to draft

@ChunkyProgrammer ChunkyProgrammer marked this pull request as ready for review September 23, 2022 17:14
ChunkyProgrammer
ChunkyProgrammer commented Sep 23, 2022
View reviewed changes
src/renderer/components/data-settings/data-settings.js
}
subscriptions.push(subscription)
const ytsubs = youtubeSubscriptions.slice(1).map(yt => {
const splitCSVRegex = /(?:,|\n|^)("(?:(?:"")*[^"]*)*"|[^",\n]*|(?:\n|$))/g
Copy link
Member Author

Choose a reason for hiding this comment

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

splitting on comma was okay for when we just wanted to get the id but it had to be changed as we would likely want to get the name (in the future) which could have commas is in it

@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 23, 2022
@ChunkyProgrammer ChunkyProgrammer marked this pull request as draft September 23, 2022 18:54
@PikachuEXE PikachuEXE marked this pull request as ready for review September 24, 2022 01:03
@PikachuEXE
Copy link
Collaborator

Oh wait you mark it as draft explicitly?

@ChunkyProgrammer @PikachuEXE
Update src/renderer/components/data-settings/data-settings.js
a4143b4 
Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
@PrestonN PrestonN enabled auto-merge (squash) September 24, 2022 03:06
@ChunkyProgrammer
Copy link
Member Author

Oh wait you mark it as draft explicitly?

Yeah, there's an issue where if you already subscribed to a channel it'll say there was an error with that channel instead of just not subscribing you

@ChunkyProgrammer ChunkyProgrammer marked this pull request as draft September 26, 2022 20:19
auto-merge was automatically disabled September 26, 2022 20:19

Pull request was converted to draft

@ChunkyProgrammer
fix unexpected errors
93a9028 
dont show toast when no errors. dont error when already subscribed
@ChunkyProgrammer
remove check for legacy subscriptions
95c9bad
@ChunkyProgrammer
rename method
c756b73
@ChunkyProgrammer ChunkyProgrammer marked this pull request as ready for review September 29, 2022 01:39
@ChunkyProgrammer
Copy link
Member Author

Should be ready for review now

ChunkyProgrammer
ChunkyProgrammer commented Sep 29, 2022
View reviewed changes
src/renderer/components/data-settings/data-settings.js
@@ -940,17 +935,6 @@ export default Vue.extend({
})
},

checkForLegacySubscriptions: async function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

Importing FreeTube subscriptions already supports importing legacy subscriptions so I don't see a need for a "CheckForLegacySubscriptions" especially since it's been 2 years since this was added

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 29, 2022
@PrestonN PrestonN enabled auto-merge (squash) September 29, 2022 01:42
ChunkyProgrammer
ChunkyProgrammer commented Sep 29, 2022
View reviewed changes
src/renderer/components/data-settings/data-settings.js Outdated
@@ -129,6 +114,25 @@ export default Vue.extend({
})
return
}
response.filePaths.forEach(file => {
if (file.endsWith('.csv')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

automatically determine subscription type instead of having the user specify it when importing

@ChunkyProgrammer ChunkyProgrammer mentioned this pull request Sep 29, 2022
Closed
@ChunkyProgrammer ChunkyProgrammer added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 29, 2022
absidue
absidue reviewed Oct 6, 2022
View reviewed changes
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.

Code looks good, will need to do a bunch of testing.

@absidue
Copy link
Member

absidue commented Oct 7, 2022

One issue I noticed is that if you have changed the colour of the name of the All channels profile and it's in the back up, it doesn't get restored when importing the backup. I also went and checked that the issue existed before this PR as well. If it's an easy fix, it might be worth including as part of this PR.

@PikachuEXE
Copy link
Collaborator

This PR has plenty of changes already
I prefer having a separate PR for the fix

@absidue
Copy link
Member

absidue commented Oct 8, 2022 •
edited
Loading

Okay yes, that makes sense, I'll approve it now then, as everything else works.

@PrestonN PrestonN merged commit 41fee01 into FreeTubeApp:development Oct 9, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 9, 2022
MarmadileManteater added a commit to MarmadileManteater/FreeTubeAndroid that referenced this pull request Oct 11, 2022
@MarmadileManteater
# Automated nightly release 67
23a8014 
commit c886559
Merge: f4d73ca 09e61ae
Author: Emma <MarmadileManteater@proton.me>

Merge branch 'development' of https://github.com/MarmadileManteater/FreeTubeCordova into development

commit f4d73ca
Merge: 0e605d9 ae9d329
Author: Emma <MarmadileManteater@proton.me>

Merge 'upstream/development' into development

commit ae9d329
Author: atilluF <atilluf@outlook.com>

Translated using Weblate (Italian)

commit b2b9d97
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump electron-builder from 23.3.3 to 23.6.0 (FreeTubeApp#2705)

commit 3c7b143
Author: Emma <MarmadileManteater@proton.me>

Improving responsiveness in Settings (FreeTubeApp#2694)

commit 3af6dd7
Author: Jeff Huang <s8321414@gmail.com>

Translated using Weblate (Chinese (Traditional))

commit b127c85
Author: Eric <hamburger1024@mailbox.org>

Translated using Weblate (Chinese (Simplified))

commit 40859b5
Author: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>

Update index.js (FreeTubeApp#2706)

commit 3db62f6
Author: gallegonovato <fran-carro@hotmail.es>

Translated using Weblate (Spanish)

commit b4bfbdc
Author: Ihor Hordiichuk <igor_ck@outlook.com>

Translated using Weblate (Ukrainian)

commit 09e61ae
Merge: 0e605d9 bdb8b17
Author: Emma <MarmadileManteater@proton.me>

Merge branch 'FreeTubeApp:development' into development

commit 0e605d9
Author: Emma <MarmadileManteater@proton.me>

Removing custom videoJS css which is no longer necessary

commit d5bc0cd
Merge: ec906ef 687352f
Author: Emma <MarmadileManteater@proton.me>

Merge remote-tracking branch 'upstream/development' into development

commit ec906ef
Merge: e202c79 7ca6440
Author: Emma <MarmadileManteater@proton.me>

Merge branch 'upstream_development' into development

commit bdb8b17
Merge: 687352f e64db7f
Author: Hosted Weblate <hosted@weblate.org>

Merge branch 'origin/development' into Weblate.

commit e64db7f
Author: Oğuz Ersen <oguz@ersen.moe>

Translated using Weblate (Turkish)

commit 66e1ff3
Author: Rex_sa <rex.sa@pm.me>

Translated using Weblate (Arabic)

commit 687352f
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump mikefarah/yq from 4.27.5 to 4.28.1 (FreeTubeApp#2703)

commit 1508b05
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump lefthook from 1.1.1 to 1.1.2 (FreeTubeApp#2699)

commit b3e0cc3
Author: Preston <freetubeapp@protonmail.com>

Switch token for Flathub deployment

commit c63149d
Merge: 7fb5d47 f07aefd
Author: Hosted Weblate <hosted@weblate.org>

Merge branch 'origin/development' into Weblate.

commit f07aefd
Author: gallegonovato <fran-carro@hotmail.es>

Translated using Weblate (Spanish)

commit 7fb5d47
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump yt-channel-info from 3.1.0 to 3.1.1 (FreeTubeApp#2702)

commit 5106868
Author: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>

Update report.yml (FreeTubeApp#2704)

commit ce24739
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump mini-css-extract-plugin from 2.6.0 to 2.6.1 (FreeTubeApp#2701)

commit 48fe90e
Merge: 22a5062 03cb0db
Author: Hosted Weblate <hosted@weblate.org>

Merge branch 'origin/development' into Weblate.

commit 03cb0db
Author: gallegonovato <fran-carro@hotmail.es>

Translated using Weblate (Galician)

commit eeeb903
Author: Florin Voicu <florin.bkk@gmail.com>

Translated using Weblate (Romanian)

commit 74f57b7
Author: HexagonCDN <ctyducliem2006@gmail.com>

Translated using Weblate (Vietnamese)

commit 22a5062
Author: absidue <48293849+absidue@users.noreply.github.com>

Move calculateColorLuminance, calculatePublishedDate and buildVTTFileLocally out of the store (FreeTubeApp#2692)

commit 7ca6440
Author: Aiz <66974576+Aiz0@users.noreply.github.com>

Add shortcuts for refresh buttons on Subscription, Trending, and Popular views (FreeTubeApp#2689)

commit aa4a01b
Author: absidue <48293849+absidue@users.noreply.github.com>

Cleanup the web webpack config (FreeTubeApp#2690)

commit 41fee01
Author: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>

Improve Importing Subscriptions (FreeTubeApp#2604)

commit 2154255
Author: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>

Advertise FT better in README (FreeTubeApp#2677)

**Full Changelog**: 0.17.1-nightly-66...0.17.1-nightly-67
Closed
1 task
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@absidue absidue absidue approved these changes

@PikachuEXE PikachuEXE PikachuEXE approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Local API error when importing youtube subscription
5 participants
@ChunkyProgrammer @PikachuEXE @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @PrestonN