[STCOR-888] RTR dynamic configuration, debugging event #1535

ncovercash · 2024-09-18T20:59:30Z

Jira STCOR-888

This PR adds:

A new event RTR_FORCE_REFRESH_EVENT that forces a RTR refresh
- This is not dispatched by stripes-core, but will aid development/debugging
Dynamic configuration for RTR_AT_TTL_FRACTION to allow customization from stripes.config.js ~~and/or at runtime via ui-developer~~

github-actions · 2024-09-18T21:01:20Z

Jest Unit Test Results

1 files ±0 56 suites ±0 1m 0s ⏱️ -1s
339 tests ±0 339 ✅ ±0 0 💤 ±0 0 ❌ ±0
345 runs +2 345 ✅ +2 0 💤 ±0 0 ❌ ±0

Results for commit 9b13e4a. ± Comparison against base commit 0e4d2b4.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.

configureRtr leaves existing settings in place ‑ configureRtr leaves existing settings in place
configureRtr sets idleSessionTTL and idleModalTTL ‑ configureRtr sets idleSessionTTL and idleModalTTL

FFetch class force refresh event Invokes a refresh on RTR_FORCE_REFRESH_EVENT... ‑ FFetch class force refresh event Invokes a refresh on RTR_FORCE_REFRESH_EVENT...
configureRtr sets default values as applicable ‑ configureRtr sets default values as applicable

♻️ This comment has been updated with latest results.

github-actions · 2024-09-18T21:02:44Z

Bigtest Unit Test Results

192 tests ±0 187 ✅ ±0 6s ⏱️ ±0s
1 suites ±0 5 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 9b13e4a. ± Comparison against base commit 0e4d2b4.

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

ncovercash · 2024-09-18T21:50:15Z

src/components/SessionEventContainer/SessionEventContainer.js

    // We only want to configure the event listeners once, not every time
    // there is a change to stripes or history. Hence, an empty dependency
    // array.
+    // should `stripes.rtr` go here?
  }, []); // eslint-disable-line react-hooks/exhaustive-deps


@zburke can you comment on this? As-is, this configuration cannot be modified at runtime by ui-developer.

Probably I started with a non-empty array and it didn't work as expected, so I added the empty array as a crutch and never looked back. Your observation/criticism is fair, but I don't have time to debug this further since the only thing "broken" about it is that it can't be modified at runtime via dev-tools.

Tested it, and it works beautifully. Only the RTR config in ui-developer causes the effect to re-run, nothing else (not even the other config forms in developer)

It does rely on not calling configureRtr on render in Root, though.

ncovercash · 2024-09-25T00:27:56Z

src/components/Root/FFetch.js

@@ -77,10 +77,27 @@ const OKAPI_FETCH_OPTIONS = {
 };

 export class FFetch {
-  constructor({ logger, store, rtrConfig }) {


RTR config is exposed as part of the main stripes-config. Why should FFetch get passed it in a non-standard way?

Strikes me as 'better testability' if you're able to just pass the config in vs having to mock it out/provide different mocks through some other way.

Fair critique. @JohnC-80, do you remember if there is a particular reason we did it this way? My bet is that we were thinking "We need to pass it through the constructor because this is a regular class, not a React component, so it won't have access to the stripes object" which is true, but Noah is correct that since we can just grab the import ... we might as well just grab the import.

zburke

Sorry for the delay; I got started on this and then lost track of it.

At a high-level the code changes look good. I would prefer to consolidate the event handlers into SessionEventContainer, but that's just ergonomics. If you merge without doing this, please explain why you opted not to.

src/components/Root/FFetch.test.js

zburke · 2024-10-03T18:33:30Z

src/components/Root/FFetch.js

@@ -77,10 +77,27 @@ const OKAPI_FETCH_OPTIONS = {
 };

 export class FFetch {
-  constructor({ logger, store, rtrConfig }) {


Fair critique. @JohnC-80, do you remember if there is a particular reason we did it this way? My bet is that we were thinking "We need to pass it through the constructor because this is a regular class, not a React component, so it won't have access to the stripes object" which is true, but Noah is correct that since we can just grab the import ... we might as well just grab the import.

zburke · 2024-10-10T17:09:25Z

src/components/Root/FFetch.js

+  /**
+   * registers a listener for the RTR_FORCE_REFRESH_EVENT
+   */
+  registerEventListener = () => {
+    this.globalEventCallback = () => {
+      this.logger.log('rtr', 'forcing rotation due to RTR_FORCE_REFRESH_EVENT');
+      rtr(this.nativeFetch, this.logger, this.rotateCallback, this.store.getState().okapi);
+    };
+    window.addEventListener(RTR_FORCE_REFRESH_EVENT, this.globalEventCallback);
+  }
+
+  /**
+   * unregister the listener for the RTR_FORCE_REFRESH_EVENT
+   */
+  unregisterEventListener = () => {
+    window.removeEventListener(RTR_FORCE_REFRESH_EVENT, this.globalEventCallback);
  }


All our other event handlers are consolidated within SessionEventContainer. Can we move this there too? If not why not?

Functionally it should not matter either way, but it would be nice to keep similar things in the same place.

Unfortunately, I don't think this can be consolidated; this handler specifically needs access to those callbacks and fields within FFetch, which SessionEventContainer has no way to access

zburke · 2024-10-10T17:13:17Z

src/components/Root/Root.js

-      const rtrConfig = configureRtr(this.props.config.rtr);
+      // FFetch relies on some of these properties, so we must ensure
+      // they are filled before initialization
+      this.props.config.rtr = configureRtr(this.props.config.rtr);


Reassigning something on props feels icky. Why was it necessary to change this line?

It's that, or reassigning the import (configureRtr doesn't change the input). This was changed because my FFetch imported config via stripes-config, whereas before it was passed in.

I can:

revert it to being passed in instead, if preferred, but IMO it's best for the shared config to match the filled-in version from configureRtr (and we already do that in render(), where we have gross: this overwrites whatever is currently stored at config.rtr)

make configureRtr mutate its input

There are two things happening here with the RTR config:

passing it to FFetch (in the constructor)

passing it Stripes (in render)

As you noted, we can get around (1) by updating FFetch to leverage stripes-config instead, and call configureRtr() in the constructor there. Should we just do the same in render() here? Part of me screams, "You should only have to call that function once! More than once means you're doing it wrong!" The other part of me is like, "Eh, if the value in stripes-config may be sparse, think of configureRtr() as getSafeRtrValues(). Does that still bug ya? No? Great."

Better, worse, just different but not actually better?

Importing stripes-config for render here might make sense, but I'm not familiar enough with the rest of this to know if that's a bad idea.

What we can (and should) do is remove the configureRtr call inside render() here. It just populates the sparse values (as you said), and there's no reason any values should be removed between renders.

zburke · 2024-10-10T17:19:21Z

src/components/SessionEventContainer/SessionEventContainer.js

    // We only want to configure the event listeners once, not every time
    // there is a change to stripes or history. Hence, an empty dependency
    // array.
+    // should `stripes.rtr` go here?
  }, []); // eslint-disable-line react-hooks/exhaustive-deps


Probably I started with a non-empty array and it didn't work as expected, so I added the empty array as a crutch and never looked back. Your observation/criticism is fair, but I don't have time to debug this further since the only thing "broken" about it is that it can't be modified at runtime via dev-tools.

sonarcloud · 2024-10-11T15:13:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[STCOR-888] RTR dynamic configuration, debugging event

9eb247e

ncovercash requested review from zburke and JohnC-80 September 18, 2024 20:59

ncovercash added 2 commits September 18, 2024 17:26

tests are nice

8313e48

token-util tests

7825038

ncovercash marked this pull request as ready for review September 18, 2024 21:41

ncovercash mentioned this pull request Sep 18, 2024

[UID-159] Additional RTR debugging and configuration options folio-org/ui-developer#431

Merged

ncovercash commented Sep 18, 2024

View reviewed changes

mweaver-ebsco approved these changes Sep 19, 2024

View reviewed changes

Re-export constants

b1dacce

ncovercash mentioned this pull request Sep 20, 2024

[STCOR-888] Export RTR constants folio-org/stripes-types#64

Open

ncovercash added 4 commits September 24, 2024 18:16

Merge? Please?

89629a2

cleanup diffs

cbd03ca

Pass config in new way in test

6e85a91

Fix event test

8fb5642

ncovercash commented Sep 25, 2024

View reviewed changes

ncovercash added 2 commits October 3, 2024 14:40

Merge branch 'master' into stcor-888

5c3b6a8

Merge branch 'master' into stcor-888

a957eb8

zburke approved these changes Oct 10, 2024

View reviewed changes

ncovercash added 5 commits October 10, 2024 13:44

Use cleaner constant sharing for FFetch test

7c63eef

Listen to changes in SessionEventContainer

f013931

fix typo

50c1291

bye bye

46b503f

fix tests

51ecc01

ncovercash mentioned this pull request Oct 10, 2024

[STCOR-898] Allow RTR configuration to be runtime-configurable #1544

Draft

Merge branch 'master' into stcor-888

9b13e4a

julianladisch requested a review from steveellis October 16, 2024 15:22

julianladisch self-requested a review October 16, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STCOR-888] RTR dynamic configuration, debugging event #1535

[STCOR-888] RTR dynamic configuration, debugging event #1535

ncovercash commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

ncovercash Sep 18, 2024

zburke Oct 10, 2024

ncovercash Oct 10, 2024

ncovercash Oct 10, 2024

ncovercash Sep 25, 2024

JohnC-80 Oct 3, 2024

zburke Oct 3, 2024

zburke left a comment

zburke Oct 3, 2024

zburke Oct 10, 2024

ncovercash Oct 10, 2024

zburke Oct 10, 2024

ncovercash Oct 10, 2024

zburke Oct 10, 2024

ncovercash Oct 10, 2024

zburke Oct 10, 2024

sonarcloud bot commented Oct 11, 2024

[STCOR-888] RTR dynamic configuration, debugging event #1535

Are you sure you want to change the base?

[STCOR-888] RTR dynamic configuration, debugging event #1535

Conversation

ncovercash commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024 • edited Loading

Jest Unit Test Results

github-actions bot commented Sep 18, 2024 • edited Loading

Bigtest Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zburke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 11, 2024

Quality Gate passed

ncovercash commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading