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

Add Accept-Language header to requests #8621

Merged
merged 4 commits into from
Mar 20, 2023
Merged

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Mar 15, 2023

Description

Adds the Accept-Language header to all outgoing requests so the server can deliver proper translations if necessary.

This includes a few refactorings:

  • The watcher on selectedLanguage in the runtime has been removed. The current language now gets set by the UserManager each time the user context is updated as well as when the user updates their language. This change was made because the UserManager needs to fetch the current language anyways as it needs to initialize the SDK with it.
  • The ClientService is now being initialized properly when bootstrapping the app, hence the singleton has been removed. Please use the useClientService composable to access the service.
  • All clients provided by the ClientService can now be accessed via a simple getter, there is not need to pass things like an accessToken every time (with ocsPublicLinkContext being the only exception because we need to pass a password).

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Follow-Ups

  • Update SDK
  • Remove useGraphClient composable

@JammingBen JammingBen self-assigned this Mar 15, 2023
@JammingBen JammingBen force-pushed the add-language-header branch 3 times, most recently from 4ce3a00 to 7a4b86e Compare March 16, 2023 10:59
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! 😍


describe('avatarUrl', () => {
it('throws an error', async () => {
const defaultOptions = getDefaultOptions()
defaultOptions.clientService.httpAuthenticated.head.mockResolvedValue({ status: 200 })
defaultOptions.clientService.owncloudSdk.signUrl.mockRejectedValue(new Error('error'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah! 😍

This is so much more readable than before.

} as any)
)
const clientService = mockDeep<ClientService>()
clientService.owncloudSdk.files.search.mockImplementation(searchMock)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Member

@dschmidt dschmidt Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value of this composable has significantly dropped with it just not having to handle access token and store. By now it only destructures the return value of useClientService().

We could/should consider to drop this composable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good point. I'll check when rebasing the PR the next time!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather do this in a follow-up, it's being used quite a few times actually.

graph: Graph
ocs: OCS
}

const createAxiosInstance = (authParams: AuthParameters): AxiosInstance => {
interface HttpClient {
client: _HttpClient
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing, but I don't have better name recommendations either

JammingBen added a commit that referenced this pull request Mar 17, 2023
@JammingBen JammingBen force-pushed the add-language-header branch from 7a4b86e to 09ed997 Compare March 17, 2023 12:53
@owncloud owncloud deleted a comment from update-docs bot Mar 17, 2023
@JammingBen JammingBen marked this pull request as ready for review March 17, 2023 13:00
JammingBen added a commit that referenced this pull request Mar 17, 2023
@JammingBen JammingBen force-pushed the add-language-header branch from 09ed997 to 89571db Compare March 17, 2023 13:00
@JammingBen JammingBen force-pushed the add-language-header branch from 89571db to 9344a3f Compare March 20, 2023 10:18
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

61.1% 61.1% Coverage
1.9% 1.9% Duplication

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, thanks a lot!

token?: string
}

const createAxiosInstance = (authParams: AuthParameters, language): AxiosInstance => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

language has no type, but we can sneak that in in the next PR no need to trigger a new CI run for this

@JammingBen JammingBen merged commit 43551a2 into master Mar 20, 2023
@delete-merged-branch delete-merged-branch bot deleted the add-language-header branch March 20, 2023 12:59
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.

[web] Always send a language header in all requests
2 participants