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

update to multi-user configs (DO NOT MERGE) #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LukePulverenti
Copy link

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:

  • Write the code that fills the UI from the UserConfig object (I left a spot for you in the code)
  • Write the code that updates the UserConfig object with data from the UI (I also left a spot)
  • I wired up the form submit event for you, but you'll need to re-add any other event handlers that you may need
  • I changed all uses of id in your html to class, which is a best practice with Emby. So be careful when porting in the javascript as that code will have to be converted to use the element class rather than id
  • The web app no longer ships with jQuery, so you'll need to convert all usage of it to plain JavaScript. This shouldn't be too hard, but just FYI.
  • You have some inline event handlers directly in the html. You'll need to remove these and add event handlers to simkl.js. Just put them in the same place as the form submit event.

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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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 the SyncHistoryAsync method to accept a UserConfig object instead of a user token.
  • UI Updates: The configuration page (configPage.html) is replaced with new HTML (simkl.html and simkladmin.html) and JavaScript (simkl.js and simkladmin.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 the GetUserSettings route and handler, streamlining the API surface.
  • Simkl-Emby/API/SimklApi.cs
    • Added IUserManager dependency to the SimklApi constructor.
    • Modified markAsWatched to accept UserConfig and embyUserId instead of just userToken.
    • Modified SyncHistoryAsync to accept UserConfig and embyUserId instead of just userToken.
    • The SyncHistoryAsync method now handles invalid user tokens by clearing the token from the UserConfig and updating the user settings.
  • Simkl-Emby/Configuration/PluginConfiguration.cs
    • Added ConfigurationFactory and SimklConfigStore to handle user-specific configurations.
    • Added a TODO to delete the userConfigs property after the upgrade period.
    • The PluginConfiguration.cs file introduces a new configuration system using IUserConfigurationFactory to manage user-specific settings, replacing the previous global configuration.
  • 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 and StaticName 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 obsolete deleteUserToken method.
  • Simkl-Emby/Services/Scrobbler.cs
    • Added IUserManager dependency to the Scrobbler constructor.
    • Modified embyPlaybackProgress to use IUserManager to get user-specific configuration.
    • Modified embyPlaybackProgress to pass UserConfig and embyUserId to markAsWatched.
    • Added UpgradeDataIfNeeded and UpgradeData 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 the IUserManager.
  • Simkl-Emby/Simkl.csproj
    • Updated AssemblyVersion and FileVersion 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.
  • 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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

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.

1 participant