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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes for client-setting, authentication-setting services hook refactor #8990

Merged
merged 11 commits into from Oct 15, 2023

Conversation

MoizAdnan
Copy link
Member

@MoizAdnan MoizAdnan commented Oct 5, 2023

Summary

馃 Generated by Copilot at a225101

Refactored the service classes and hooks for authentication and client settings in server-core package. Used KnexService instead of KnexAdapter for database access, moved custom logic to hooks, and simplified imports and arguments. These changes improve the code quality, modularity, and maintainability of the settings modules.

References

#8871

Explanation

馃 Generated by Copilot at a225101

  • Refactored the AuthenticationSettingService and ClientSettingService classes to extend the KnexService class instead of the KnexAdapter class, which is deprecated (link, link)
  • Simplified and updated the imports in the service class files to use only the necessary types and modules (link, link, link)
  • Removed the custom methods for find, get, and patch from the service class files and moved them to the hooks files, where they can be applied as middleware functions (link, link, link, link, link, link)
  • Added the ensureOAuth, mapSettingsAdmin, and refreshAPIPods functions to the authentication-setting.hooks.ts file, which are custom hooks that perform additional logic before or after the service methods for the AuthenticationSettingService (link, link, link)
  • Added the updateWebManifest function to the client-setting.hooks.ts file, which is a custom hook that updates the web manifest file in the storage provider with the new client settings data when the patch method is called for the ClientSettingService (link, link, link)
  • Updated the app.use calls in the authentication-setting.ts and client-setting.ts files to pass only the options argument to the service class constructors, since the app argument is no longer needed (link, link)

馃 Generated by Copilot at a225101

Oh we're the coders of the server core
And we refactor as we go
We use the KnexService and the hooks galore
To make our code more so

QA Steps

List any additional steps required to QA the changes of this PR, as well as any supplemental images or videos.

Checklist

  • If this PR is still a WIP, convert to a draft
  • When this PR is ready, mark it as "Ready for review"
  • ensure all checks pass
  • Changes have been manually QA'd
  • Changes reviewed by at least 2 approved reviewers

@MoizAdnan MoizAdnan marked this pull request as ready for review October 9, 2023 04:01
@hanzlamateen hanzlamateen self-requested a review October 10, 2023 15:57
@barankyle barankyle added this pull request to the merge queue Oct 15, 2023
Merged via the queue into dev with commit 17632d0 Oct 15, 2023
13 checks passed
@barankyle barankyle deleted the settings-hooks branch October 15, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants