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

STCOR-942: migrate away from reading stripes-config::okapi.url #1589

Merged
merged 10 commits into from
Feb 13, 2025

Conversation

aidynoJ
Copy link
Contributor

@aidynoJ aidynoJ commented Feb 5, 2025

Refs STCOR-942.

@aidynoJ aidynoJ requested a review from a team as a code owner February 5, 2025 09:14
Copy link

github-actions bot commented Feb 5, 2025

Bigtest Unit Test Results

189 tests  ±0   184 ✅ ±0   6s ⏱️ ±0s
  1 suites ±0     5 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 90bcd74. ± Comparison against base commit c800a5a.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both.
      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Feb 5, 2025

Jest Unit Test Results

  1 files  ±0   62 suites  ±0   1m 32s ⏱️ ±0s
365 tests +1  365 ✅ +1  0 💤 ±0  0 ❌ ±0 
368 runs  +1  368 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 90bcd74. ± Comparison against base commit c800a5a.

♻️ This comment has been updated with latest results.

@aidynoJ aidynoJ marked this pull request as draft February 5, 2025 11:00
@aidynoJ aidynoJ marked this pull request as ready for review February 6, 2025 10:35
@aidynoJ aidynoJ requested review from ryandberger and zburke February 6, 2025 10:36
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 882 to 890
if (store.getState().okapi.authnUrl) {
return fetch(store.getState().okapi.authnUrl, {
method: 'POST',
body: new URLSearchParams({
...data,
'grant_type': 'password',
'client_id': okapi.clientId,
'client_secret': okapi.clientSecret,
'client_id': store.getState()?.okapi?.clientId,
})
})
Copy link
Member

Choose a reason for hiding this comment

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

In fact this whole conditional is irrelevant, left-over from a very early implementation. Now, when okapi.authnUrl is present, we redirect to Keycloak to authenticate, meaning we don't need a conditional at all because requestLogin is only called in legacy context.

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 see, thank you!

@aidynoJ aidynoJ merged commit fa2ffcf into master Feb 13, 2025
16 checks passed
@aidynoJ aidynoJ deleted the STCOR-942 branch February 13, 2025 13:42
zburke added a commit that referenced this pull request Feb 14, 2025
Fetch `tenant` from state instead of from `stripes-config`. This
resolves a bug introduced in #1589 that left a dangling reference to
`this.tenant` that was no longer populated.

Refs STCOR-942
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.

2 participants