-
Notifications
You must be signed in to change notification settings - Fork 5
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
update to multi-user configs (DO NOT MERGE) #18
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @LukePulverenti, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on updating the Simkl Emby plugin to support multi-user configurations. The primary goal is to transition the plugin from a global configuration to a user-specific configuration, allowing each Emby user to manage their own Simkl settings. This involves changes to the server-side API endpoints, data storage, and the user interface. The author, LukePulverenti, has completed most of the server-side work and has outlined the remaining JavaScript tasks in simkl.js
for the reviewer.
Highlights
- Multi-User Configuration: The plugin is being updated to support individual Simkl configurations for each Emby user, moving away from a single global configuration.
- API Endpoint Changes: The pull request removes the
GetUserSettings
API endpoint and modifies theSyncHistoryAsync
method to accept aUserConfig
object instead of a user token. - UI Updates: The configuration page (
configPage.html
) is replaced with new HTML (simkl.html
andsimkladmin.html
) and JavaScript (simkl.js
andsimkladmin.js
) files to handle user-specific settings. The old config page is removed. - Data Migration: Includes logic to migrate existing global configurations to user-specific configurations. A
TODO
has been added to delete this logic after a reasonable upgrade period. - Dependency Updates: The pull request updates the NuGet package to version 4.8.0.20 and removes jQuery dependency from the web app.
Changelog
Click here to see the changelog
- Simkl-Emby/API/ServerEndpoint.cs
- Removed the
GetUserSettings
API endpoint. - The file
Simkl-Emby/API/ServerEndpoint.cs
has been modified to remove theGetUserSettings
route and handler, streamlining the API surface.
- Removed the
- Simkl-Emby/API/SimklApi.cs
- Added
IUserManager
dependency to theSimklApi
constructor. - Modified
markAsWatched
to acceptUserConfig
andembyUserId
instead of justuserToken
. - Modified
SyncHistoryAsync
to acceptUserConfig
andembyUserId
instead of justuserToken
. - The
SyncHistoryAsync
method now handles invalid user tokens by clearing the token from theUserConfig
and updating the user settings.
- Added
- Simkl-Emby/Configuration/PluginConfiguration.cs
- Added
ConfigurationFactory
andSimklConfigStore
to handle user-specific configurations. - Added a
TODO
to delete theuserConfigs
property after the upgrade period. - The
PluginConfiguration.cs
file introduces a new configuration system usingIUserConfigurationFactory
to manage user-specific settings, replacing the previous global configuration.
- Added
- Simkl-Emby/Configuration/configPage.html
- Removed the old configuration page.
- Simkl-Emby/Configuration/simkl.html
- Added new HTML file for user-specific configuration page.
- Simkl-Emby/Configuration/simkl.js
- Added new JavaScript file to handle user-specific configuration logic.
- The
simkl.js
file implements the client-side logic for the new user-specific configuration page, including fetching, updating, and saving user settings.
- Simkl-Emby/Configuration/simkladmin.html
- Added new HTML file to inform admins about the move to user-specific settings.
- Simkl-Emby/Configuration/simkladmin.js
- Added new JavaScript file for the admin information page.
- Simkl-Emby/Plugin.cs
- Added
StaticId
andStaticName
properties. - Modified
GetWebPages
to include new HTML and JavaScript files. - Removed
deleteUserToken
method. - The
Plugin.cs
file has been updated to register new web pages for user-specific settings and to remove the obsoletedeleteUserToken
method.
- Added
- Simkl-Emby/Services/Scrobbler.cs
- Added
IUserManager
dependency to theScrobbler
constructor. - Modified
embyPlaybackProgress
to useIUserManager
to get user-specific configuration. - Modified
embyPlaybackProgress
to passUserConfig
andembyUserId
tomarkAsWatched
. - Added
UpgradeDataIfNeeded
andUpgradeData
methods to handle data migration from global to user-specific configurations. - The
Scrobbler.cs
file has been updated to use the new user-specific configuration system, including data migration and integration with theIUserManager
.
- Added
- Simkl-Emby/Simkl.csproj
- Updated
AssemblyVersion
andFileVersion
to 1.0.8.0. - Updated
MediaBrowser.Server.Core
package reference to 4.8.0.20-beta10. - Added new HTML and JavaScript files as embedded resources.
- Updated
- Simkl-Emby/SimklFeature.cs
- Added new file to register a feature in user permissions for each user, so that admins can restrict use of it
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The term "scrobbling," as used in the context of media tracking, originated with the Last.fm music service.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant changes to the Simkl plugin, including migrating user configurations to user-specific settings and updating the UI. The changes aim to improve the plugin's usability and maintainability. However, careful attention should be paid to the data migration process, JavaScript conversion, and UI updates to ensure a smooth transition and prevent any issues for existing users.
Summary of Findings
- Data Migration: The code includes a data migration step to move user configurations from the plugin configuration to user-specific settings. Ensure this migration is robust and handles potential errors gracefully. Consider adding logging to track the migration process and provide insights into its success or failure.
- Removal of Old Configuration: The code includes a TODO to remove the old plugin configuration after a migration period. Ensure this is done to avoid confusion and potential conflicts with the new user-specific settings.
- JavaScript Conversion: The pull request description mentions converting jQuery usage to plain JavaScript. Ensure all instances of jQuery have been replaced and the new code functions correctly across different browsers.
Merge Readiness
The pull request is not ready for merging due to the presence of TODOs and the need for thorough testing of the data migration and JavaScript conversion. The data migration needs to be robust and handle potential errors gracefully. The JavaScript conversion needs to be verified to ensure all instances of jQuery have been replaced and the new code functions correctly across different browsers. I am unable to approve this pull request, and users should have others review and approve this code before merging.
Do not merge because there is still more work to do, however, this gets you most of the way there. I believe I have all of the server-side work covered for you, but search the code base for where I added TODO.
The area that still needs work is the JavaScript in simkl.js.
You will need to:
Also I updated the nuget package to 4.8.0.20, so once you finally submit to the Emby plugin catalog, that will be your min required server version. But you'll probably want to update that to 4.8.0.80 since that was a stable package release.