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

Added view of synced subs in personal settings #85

Merged
merged 35 commits into from Oct 28, 2022

Conversation

applejag
Copy link
Contributor

@applejag applejag commented Jul 17, 2022

  • PHP changes:

    • New endpoints:
      • GET apps/gpoddersync/personal_settings/metrics
      • GET apps/gpoddersync/personal_settings/podcast_data
    • New controller: lib/Controller/PersonalSettingsController.php
    • New types in lib/Core/PodcastData
    • Added personal settings page to appinfo/info.xml that points to lib/Settings/GPodderSyncPersonal.php, that in turn just points to the Vue view.
  • Added Vue! I've basically no idea what I'm doing, but I took a lot of inspiration from:

  • CI/CD:

    • Updated release GitHub action to also build JavaScript code
    • Added GitHub Action for building JavaScript code on all pull requests

Closes #88

Screenshots

2022-07-17_20-34
2022-07-17_20-30

Disclaimer

I've not used PHP in so many years, and this is my first time with Vue.

Therefore, please go ahead and roast my code as much as you can 🔥 I can take it, and more importantly I want to learn what others want to focus on to keep their PHP and Vue code clean and maintainable.

@thrillfall
Copy link
Owner

Thanks for this great contribution!
I love that you kept your commit history. I love that you did the vue part though "not having any idea". That's the spirit 😛

@thrillfall
Copy link
Owner

thrillfall commented Jul 19, 2022

Since I am on vacation I don't want to spent time to do a thorough review which does not mean that we cannot merge this soon. What we definitely need is tests. I left you a comment in the controller class

@applejag
Copy link
Contributor Author

applejag commented Jul 19, 2022

Since I am on vacation I don't want to spent time to do a thorough review which does not mean that we cannot merge this soon. What we definitely need is tests. I left you a comment in the controller class

Don't sweat it. This PR is not a prio. Enjoy your vacation 😊 I'm completly fine with this PR taking months

@JonOfUs
Copy link
Collaborator

JonOfUs commented Aug 7, 2022

I really like this feature! Huge upgrade to the app while not changing anything in the DB.

I sent you some code for csp exceptions, because on my instance podcast images wouldn't load.

What would you think of a sorted podcast view, e.g. descending by play time? Or a little box where one can select a sorting method?

@applejag
Copy link
Contributor Author

applejag commented Aug 7, 2022

I really like this feature! Huge upgrade to the app while not changing anything in the DB.

I sent you some code for csp exceptions, because on my instance podcast images wouldn't load.

What would you think of a sorted podcast view, e.g. descending by play time? Or a little box where one can select a sorting method?

I've been putting off updating this for a while 😅 but yea adding sorting is kind of a given feature that this should have. Nice suggestion!

Regarding the CSR, just letting the server act as a proxy would work right? Having an endpoint of e.g /apps/gpoddersync/personal_settings/podcast/{id}/image that just returns the image BLOB. That way we could also add a bunch of caching headers so the client doesn't ask for it again, while it should in theory also prevent SSRF. Caching it on the server could also be good.

@JonOfUs
Copy link
Collaborator

JonOfUs commented Aug 7, 2022

Yes, using the server as proxy should work without CSP exceptions and probably even increase speed (and reduce attack vectors). But where should the cached images be stored? Is a OCP\ICache a good solution for images? Never used it...

All good, I just had the time to look at the PR since it looks so interesting. You can close my PR or leave it until further updating, only noticed it while testing.

@thrillfall
Copy link
Owner

@jilleJr looking forward to this. Can we support you somehow?

@applejag
Copy link
Contributor Author

I've been mostly spending my free time on a different PR in a different project, but that has been merged now, so now I can finally get back to this one. I would love to at least attempt extracting the code out into testable code myself, and if that goes badly I'll ask for some help :)

@applejag
Copy link
Contributor Author

Yea actually one things I would love help with, do any of you know a good guide for setting up a local Nextcloud for developing apps? I'm struggling so much trying to follow the official documentation on this (https://docs.nextcloud.com/server/latest/developer_manual/getting_started/devenv.html). The docs suggest how to set up Nextcloud for dev purposes in so many different ways, and so I've resulted in a frankenstein installation that tries a bunch of different paths they suggest in the docs.

Would love a clear step by step without "choose git or tarball, choose apache or nginx, choose to store it in /var/www or some other dir, choose to chown as www-data or your own user". I need a guide on how to set up a nextcloud without deviating docs on all the possibilities 😅


For a status update, I got some coding in today where I've successfully set up VS Code for some intellisense so I could more comfortable do some larger refactoring. I'm putting the PR as "draft" in the meantime, as I still have the tests to implement.

@applejag applejag marked this pull request as draft August 28, 2022 19:29
@applejag
Copy link
Contributor Author

Okay now I'm ready!

I've overhauled how it works a bit. There's now two endpoints instead of just one:

  • GET /personal_settings/metrics
  • GET /personal_settings/podcast_data (new)

The former only returns metrics that are solely obtained from the database.

The latter does the RSS request in the backend, parses the XML, and also requests the image (to solve the Content Security Policy issue), if any.

The frontend also got a "sort by listened time" dropdown. It's not sorting by anything else for now, to keep it simple.


Future improvements, that I think are best added in a different PR:

  • More sorting options, e.g sort by name, when the podcast was first subscribed to, etc.
  • Add actions to the dropdown on each podcast in the list, such as:
    • Merge with other podcast (to resolve duplicates)
    • Remove podcast/unsubscribe
    • Remove episode tracking/clear listened status
  • Add "Subscribe to URL" form to add new podcasts
  • Add button to reset the webpage podcast cache (mostly only needed for debugging purposes)
  • Add "Export" download and "Import" file upload to move the database's state
  • Add "Import from other gpodder server" procedure

Lots of fun ideas to add to the settings panel :)

@applejag applejag marked this pull request as ready for review September 17, 2022 19:58
@JonOfUs
Copy link
Collaborator

JonOfUs commented Sep 18, 2022

Wow, just installed it on a test server with caching and after first load it's insanely fast :D and looks very good

Copy link
Collaborator

@JonOfUs JonOfUs left a comment

Choose a reason for hiding this comment

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

Did my best in finding any flaws in your code.
Sadly, I am not very experienced in php or vue, so I can't critizise your coding style. Just marked some things that could be removed or that could lead to problems.

lib/Controller/PersonalSettingsController.php Outdated Show resolved Hide resolved
lib/Controller/PersonalSettingsController.php Outdated Show resolved Hide resolved
lib/Controller/PersonalSettingsController.php Outdated Show resolved Hide resolved
lib/Controller/PersonalSettingsController.php Outdated Show resolved Hide resolved
src/views/PersonalSettingsPage.vue Outdated Show resolved Hide resolved
src/components/SubscriptionListItem.vue Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastDataReader.php Show resolved Hide resolved
src/components/SubscriptionListItem.vue Outdated Show resolved Hide resolved
@thrillfall
Copy link
Owner

i can't see the personal settings?!? Do i need to do anything else after checking out your branch?

@JonOfUs
Copy link
Collaborator

JonOfUs commented Sep 21, 2022

The .js files have to be built first:

  1. composer i for installing php dependencies (phpunit)
  2. make npm-init for installing production npm packages
  3. make build-js-production for building js files

After running these commands (successfully), the settings should appear. I had some problems installing dependencies when using Debian (probably old npm version), but Ubuntu 22.* installed everything on first try.

For developing, make dev-setup and make build-js or make watch-js are more useful than 2. and 3., though. I got the commands from the build_release action.

edit: make appstore generates this file: gpoddersync.tar.gz (if you don't want to install npm just for testing) (on commit 7b58db14b8ba103bc3b30733a54e77877affe3ee)

@thrillfall
Copy link
Owner

@jilleJr sorry but i just don't find the time for refactoring the vue components. If you are interested why I would refactor please read this: https://itnext.io/https-medium-com-manuustenko-how-to-avoid-solid-principles-violations-in-vue-js-application-1121a0df6197

If @JonOfUs can verify that this works i'd be glad to merge this

@applejag
Copy link
Contributor Author

@thrillfall
sorry but i just don't find the time for refactoring the vue components. If you are interested why I would refactor please read this: https://itnext.io/https-medium-com-manuustenko-how-to-avoid-solid-principles-violations-in-vue-js-application-1121a0df6197

I also don't find the time. So if you're ok with waiting then let's wait. I mostly only have time and energy during weekends atm. But if you're keen on releasing this then you could merge and create a refactoring issue assigned to me.

That blog post looks nice, will read through it thoroughly later. But the author didnt get rid of the fetch call from mounted, but will experiment a little to get around that. That feels like the hardest part (to the uninitiated, aka me)

@thrillfall
Copy link
Owner

@jilleJr I'd go for merging it now. But please rebase first

Copy link
Collaborator

@JonOfUs JonOfUs left a comment

Choose a reason for hiding this comment

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

Tests failed with php7.4 since named arguments are only available since php8.0

edit: and it should work without the named argument since the function only has 2 params:

parseRssXml(string $xmlString, ?int $fetchedAtUnix = null): PodcastData

tests/Unit/Core/PodcastData/PodcastDataTest.php Outdated Show resolved Hide resolved
tests/Unit/Core/PodcastData/PodcastDataTest.php Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Flueren <11487762+JonOfUs@users.noreply.github.com>
@applejag
Copy link
Contributor Author

Tests failed with php7.4 since named arguments are only available since php8.0

Oops, didn't realize. Nice that you have such a thorough test suite.

Copy link
Collaborator

@JonOfUs JonOfUs left a comment

Choose a reason for hiding this comment

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

I hope I got all of the named parameters and trailing commas. Sadly, this is not enough. I didn't know php8 brought so many new features.
The mixed type also throws errors, so that needs to be replaced as well in order for this PR to work with php7.4.

TypeError: Argument 1 passed to OCA\GPodderSync\Core\PodcastData\PodcastData::stringOrNull() must be an instance of OCA\GPodderSync\Core\PodcastData\mixed, instance of SimpleXMLElement given, called in /home/jon/nextcloud/apps/gpoddersync/lib/Core/PodcastData/PodcastData.php on line 45

lib/Core/PodcastData/PodcastData.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastData.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastMetricsReader.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@JonOfUs JonOfUs left a comment

Choose a reason for hiding this comment

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

I just installed php7.4 and tested it (not only phpunit, but actual usage). With these fixes, it worked for me. Please suggest a better alternative if you know one, e.g. for the mixed type.

lib/Controller/PersonalSettingsController.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastData.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastData.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastDataReader.php Outdated Show resolved Hide resolved
lib/Core/PodcastData/PodcastMetrics.php Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Flueren <11487762+JonOfUs@users.noreply.github.com>
@applejag
Copy link
Contributor Author

I just installed php7.4 and tested it (not only phpunit, but actual usage). With these fixes, it worked for me.

Thank you for the effort!

Please suggest a better alternative if you know one, e.g. for the mixed type.

I don't

@JonOfUs
Copy link
Collaborator

JonOfUs commented Oct 27, 2022

Thank you so much for this cool settings screen, I really like it!

@thrillfall merging has my approval, tested with some requests and various website visits on NC24+25 and php7.4+8.0. I would just go with a rebase merge, or would you prefer a squash?
I can create a PR with changelog and version bump for v3.6.0 as soon as this is merged.

@thrillfall thrillfall merged commit 3a84b25 into thrillfall:main Oct 28, 2022
@thrillfall
Copy link
Owner

Awesome! You are great! Thanks!

@applejag applejag deleted the feature/personal-settings branch December 29, 2022 08:58
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.

None yet

3 participants