nextcloud-gpodder icon indicating copy to clipboard operation
nextcloud-gpodder copied to clipboard

Added view of synced subs in personal settings

Open applejag opened this issue 3 years ago • 10 comments

  • PHP changes:

    • Added endpoint: /apps/gpoddersync/personal_settings/metrics via new lib/Controller/PersonalSettingsController.php
    • Added service lib/Service/PodcastCacheService.php that fetches the RSS XML and extracts metadata.
    • 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:

    • https://docs.nextcloud.com/server/latest/developer_manual/app_development/tutorial.html
    • https://github.com/nextcloud/app-tutorial
    • I only used existing components from https://nextcloud-vue-components.netlify.app/ so it's really basic. But it'll do for a first iteration I think :)
  • 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 :fire: 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.

applejag avatar Jul 17 '22 18:07 applejag

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 avatar Jul 19 '22 21:07 thrillfall

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

thrillfall avatar Jul 19 '22 21:07 thrillfall

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

applejag avatar Jul 19 '22 22:07 applejag

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?

JonOfUs avatar Aug 07 '22 13:08 JonOfUs

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 :sweat_smile: 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.

applejag avatar Aug 07 '22 13:08 applejag

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.

JonOfUs avatar Aug 07 '22 14:08 JonOfUs

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

thrillfall avatar Aug 24 '22 07:08 thrillfall

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 avatar Aug 24 '22 07:08 applejag

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 :sweat_smile:


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 avatar Aug 28 '22 19:08 applejag

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 avatar Sep 17 '22 19:09 applejag

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

JonOfUs avatar Sep 18 '22 13:09 JonOfUs

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

thrillfall avatar Sep 21 '22 13:09 thrillfall

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)

JonOfUs avatar Sep 21 '22 13:09 JonOfUs

@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

thrillfall avatar Oct 24 '22 17:10 thrillfall

@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)

applejag avatar Oct 24 '22 19:10 applejag

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

thrillfall avatar Oct 25 '22 06:10 thrillfall

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.

applejag avatar Oct 26 '22 20:10 applejag

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

applejag avatar Oct 27 '22 07:10 applejag

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.

JonOfUs avatar Oct 27 '22 21:10 JonOfUs

Awesome! You are great! Thanks!

thrillfall avatar Oct 28 '22 07:10 thrillfall