simjanos-dev
Basic Multi-User Setup
This is not ready to merge. I haven't quite finished/tested properly yet and will get to the rest later this week. But I thought I'd get this pull request up so you can take a look and let me know if you want anything done differently.
Main things are:
Added an endpoint to check if the current user is an admin.
This might have been better done as part of the general 'get user data' endpoint and then we could just check the is_admin property specifically whenever needed. However I see there's no global state and didn't want to pass data between components too much. This endpoint is currently only used on the admin settings page (in case the user navigates there manually it will just show an error).
Jellyfin default enabled setting
Currently the jellyfin 'enabled' toggle defaults to false. This may be annoying for users that are currently using Jellyfin (they'd probably have to go re-enable it). Not sure if you have any specific way you want to solve that? Of course we could just default it to true, but seems odd to be enabled when they haven't added their api key etc.
Middleware for admin sections
I've added a middleware to protect the admin sections. There are various other endpoints that should be moved into that middleware section - but I know I'd miss plenty so figured perhaps better if someone more familiar with all the routes did this.
Lastly, I have no idea how the anki stuff works (haven't looked at all yet) - I've not used this feature personally. Let me know if there's anything I should know for that (or if someone else wants to take over that part)
Great thanks, made those changes.
Code is all clear and easy to work on! Main thing I noticed though is that all api routes are in the web routes file. Not an issue but when creating middleware that returns a response we have to be careful whether it's a JSON or HTML response based on which endpoints it's used on. Splitting up the routes makes it easier to reason about that.
I have used Anki before - I will go ahead doing the Anki stuff if there's no rush, after I set it up on my live deployment which I should probably do anyway :)
Main thing I noticed though is that all api routes are in the web routes file
Currently refactoring my controllers #103. In the files that are already complete, they (should) all return JSON responses, except the Home controller's index which is the actual page.
Anki
I reread my first description, I don't think I explained it well. The problem is that currently when a user sends a card to Anki, the browser sends a request to the server, and the server sends it to Anki. But if there are multiple users, it doesn't make sense, so instead their own browser should send the Anki requests to their own Anki program. But this way the logic must also move from the server to the client.
Actually, just realized that there might be a problem. If we make this change, the client will have to have access to the Anki program's network. So if someone hosts linguacafe, they won't be able to send cards to Anki remotely through the linguacafe server, the client itself will have to have acces to Anki's network.
I think this is an acceptable compromise, but not sure.
@jacovanc
Hi!
I will release v0.13 sometime in June, probably mid-end June. After that I will update to Laravel 11, which would make this opened PR very hard to manage.
If you would like to, we could cut multi user feature into multiple parts. We can finish disabling Jellyfin (on the import UI), and I can add the admin routes to the middleware, then do the rest later after it was migrated to Laravel 11.
I do not want to hurry you, I just want to figure out what to do to avoid conflicts for opened PR-s. If you would like, I can also finish the rest of the feature.
Thank you for working on this, I really appreciate all contributions! And I hope you enjoy LinguaCafe!
Hey - yeah sorry I don't have much time to work on this as stuff has come up. If you can finish it that would be great. I agree that leaving the Anki part until later is probably the right move anyway, as it's a bigger feature to update that logic anyway.
Let me know if you have any questions about my changes though!
Hey - yeah sorry I don't have much time to work on this as stuff has come up. If you can finish it that would be great.
That's completely okay, I'l finish it.
Well. That was the hardest conflict resolution of my life. In the end I made it work, but has a lot of extra commits now.
Anyways, thank you so much for the PR, I really appreciate it! I'll finish disabling Jellyfin on the UI, and move the admin routes into the middleware. I'll add a warning about the rest of the problems, they can be fixed later.