STCOR-936

JohnC-80 · 2025-01-27T20:19:00Z

This PR Implements 2 things -

A centralized mechanism for managing user preferences and interacting with mod-settings.
A context that reads/writes a persisted user preference for the order of application icons in the main navigation of the UI.

1 is fairly straightforward. A hook - usePreferences - that accepts an object of options with the key and scope of a user preference. It will automatically apply the logged in user's id as well as maintain the id of an existing preference in its internal state. As per typical, it try/catches the requests. TODO: Better error handling than console.log.

usePreferences supplies methods getPreference, setPreference, and deletePreference.

getPreference/setPreference queries the preferences with the userId, scope, and key. If the preference is found, it will grab the id and store that in local state to use when setPreference is called. If a preference is not found, then the id remains null and when setPreference is called, a new preference is created according to mod-settings' API. deletePreference removes the preference and resets the found preference id if there is any.

2 involved setting up a context with a Provider - AppOrderProvider- that wraps at a high level - outside of the main navigation and around the ui-module container. It access the user preference and provides an ordered list of apps filtered by user permissions (named apps). It also provides the persisted preference value - an array containing objects with app information - only name as well as an isNew field if a platform app does not have a corresponding list order value.

The app list -building logic was lifted from MainNav.js and <MainNav> refactored as a functional component, trading out HOC's for hooks. One detail that's subject to further inspection is line 58 where it uses logout instead of an apparently undefined returnToLogin function.

ResizeContainer and AppListDropdown were two components that had to be touched as they were forcing alphabetical order of the list - switching from Object.keys to using the order of the array of apps/preference.

Still Remaining:

Tests!
Cleanup!
Anything else that comes up in review...

github-actions · 2025-01-27T20:22:16Z

Bigtest Unit Test Results

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

Results for commit 0c6e189. ± Comparison against base commit 2049911.

This pull request removes 191 and adds 189 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_131_0_0_0_(Linux_x86_64).AppIcon Passing a string to the tag-prop ‑ AppIcon Passing a string to the tag-prop Should render an AppIcon with a HTML tag of "div"
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Passing a string using the children-prop ‑ AppIcon Passing a string using the children-prop Should render an AppIcon with a label
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using Stripes-context ‑ AppIcon Rendering an AppIcon using Stripes-context Should render an <img>
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using Stripes-context ‑ AppIcon Rendering an AppIcon using Stripes-context Should render an img with an alt-attribute
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render an <img>
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render an img with an alt-attribute
Chrome_131_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render with a className of "My className"
…

Chrome_132_0_0_0_(Linux_x86_64).AppIcon Passing a string to the tag-prop ‑ AppIcon Passing a string to the tag-prop Should render an AppIcon with a HTML tag of "div"
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Passing a string using the children-prop ‑ AppIcon Passing a string using the children-prop Should render an AppIcon with a label
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using Stripes-context ‑ AppIcon Rendering an AppIcon using Stripes-context Should render an <img>
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using Stripes-context ‑ AppIcon Rendering an AppIcon using Stripes-context Should render an img with an alt-attribute
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render an <img>
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render an img with an alt-attribute
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Rendering an AppIcon using an icon-object ‑ AppIcon Rendering an AppIcon using an icon-object Should render with a className of "My className"
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Size tests Passing a size of "large" ‑ AppIcon Size tests Passing a size of "large" Should render an icon into a large-sized container
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Size tests Passing a size of "medium" ‑ AppIcon Size tests Passing a size of "medium" Should render an icon into a medium-sized container
Chrome_132_0_0_0_(Linux_x86_64).AppIcon Size tests Passing a size of "small" ‑ AppIcon Size tests Passing a size of "small" Should render an icon into a small-sized container
…

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

Chrome_131_0_0_0_(Linux_x86_64).PasswordValidationField with an invalid password ‑ PasswordValidationField with an invalid password shows a validation error
Chrome_131_0_0_0_(Linux_x86_64).useCustomFields hook requests for existing custom fields ‑ useCustomFields hook requests for existing custom fields should have custom fields
Chrome_131_0_0_0_(Linux_x86_64).useCustomFields hook requests for non-existing custom fields ‑ useCustomFields hook requests for non-existing custom fields should have error
Chrome_131_0_0_0_(Linux_x86_64).useCustomFields hook requests for version-compatible custom fields ‑ useCustomFields hook requests for version-compatible custom fields should have custom fields
Chrome_131_0_0_0_(Linux_x86_64).useCustomFields hook requests for version-incompatible custom fields ‑ useCustomFields hook requests for version-incompatible custom fields should have error

Chrome_132_0_0_0_(Linux_x86_64).PasswordValidationField with an invalid password ‑ PasswordValidationField with an invalid password shows a validation error
Chrome_132_0_0_0_(Linux_x86_64).useCustomFields hook requests for existing custom fields ‑ useCustomFields hook requests for existing custom fields should have custom fields
Chrome_132_0_0_0_(Linux_x86_64).useCustomFields hook requests for non-existing custom fields ‑ useCustomFields hook requests for non-existing custom fields should have error
Chrome_132_0_0_0_(Linux_x86_64).useCustomFields hook requests for version-compatible custom fields ‑ useCustomFields hook requests for version-compatible custom fields should have custom fields
Chrome_132_0_0_0_(Linux_x86_64).useCustomFields hook requests for version-incompatible custom fields ‑ useCustomFields hook requests for version-incompatible custom fields should have error

♻️ This comment has been updated with latest results.

github-actions · 2025-01-27T20:22:22Z

Jest Unit Test Results

1 files ±0 64 suites +2 1m 34s ⏱️ -2s
370 tests +6 357 ✅ - 7 0 💤 ±0 13 ❌ +13
373 runs +6 360 ✅ - 7 0 💤 ±0 13 ❌ +13

For more details on these failures, see this check.

Results for commit 0c6e189. ± Comparison against base commit 2049911.

♻️ This comment has been updated with latest results.

zburke · 2025-01-27T22:41:57Z

src/components/MainNav/MainNav.js

-        this.returnToLogin();
+        // this.returnToLogin();
+        logout(state.okapi.url, store, queryClient);
      }


In RTR-land where tokens only exist in HTTPOnly cookies, we don't have to worry about this condition because state.okapi.token will never evaluate to true. IOW, I think the right clean-up here is "remove this entire conditional".

It looks like the bad commit is a9b860dfe591f7e7b236640fc1ceb6d25401b30c, itself a "clean-up" commit from mega-merge PR #1532. The comment there alludes to fixing up logout, but unfortunately doesn't offer any details about why, specifically, returnToLogin was removed. If anybody knows this @zburke character we should find him and tell him to write better commit messages.

…into STCOR-936

MikeTaylor

I've not read every line of this, but it looks good.

MikeTaylor · 2025-01-28T17:13:45Z

package.json

+      },
+      {
+        "permissionName": "mod-settings.owner.read.stripes-core.prefs.manage",
+        "displayName": "UI: read the user's own preference for the order of links in the main navigation.",


Could we say something like "UI: read the user's own preferences, such as the order of links in the main navigation"? We are surely going to want to use this for lots of other things, too, once it's in place.

MikeTaylor · 2025-01-28T17:13:55Z

package.json

+      },
+      {
+        "permissionName": "mod-settings.owner.write.stripes-core.prefs.manage",
+        "displayName": "UI: update the user's own preference for the order of links in the main navigation.",


See previous comment.

MikeTaylor · 2025-01-28T17:14:59Z

src/components/MainNav/AppList/AppList.js

@@ -91,7 +91,7 @@ class AppList extends Component {
        {
          apps.map(app => {
            const isHidden = hiddenItemIds.includes(app.id);
-
+            // const isHidden = false;


What's this about?

MikeTaylor · 2025-01-28T17:24:03Z

src/components/MainNav/AppOrderProvider.js

+    const id = `clickable-${name}-module`;
+
+    const pathRoot = pathname.split('/')[1];
+    const entryRoot = entry.route.split('/')[1];


It looks this means each FOLIO UI module has to have a unique top-level path component — e.g. you can't have ui-foobar that uses /foo/bar and ui-foobaz that uses /foo/baz. Am I reading that right? If so, is that limitation new, or was it there in the old code?

Good question, @MikeTaylor. It is not a new limitation; old code is identical and hasn't changed in 7+ years. Before that, history is too murky to easily assess, so I don't know if the limitation has always been there but certainly it's been there for a long time.

MikeTaylor · 2025-01-28T17:29:27Z

src/components/MainNav/AppOrderProvider.js

+  const { getPreference, setPreference, removePreference } = usePreferences();
+
+  const { data: userAppList, isLoading, refetch: refetchAppList } = useQuery('UserPrefsAppListing', () => {
+    return getPreference({ key: APPORDER_PREF_NAME, scope: APPORDER_PREF_SCOPE });


Maybe use an expression here rather than a block that does nothing but return the expression?

MikeTaylor · 2025-01-28T17:36:35Z

src/hooks/usePreferences.js

+        respJSON = await resp.json();
+        if (respJSON.items.length === 1) {
+          setId(respJSON.items[0].id);
+        }


else ... ?

MikeTaylor · 2025-01-28T17:39:02Z

src/hooks/usePreferences.js

+  const userId = user.user.id;
+
+  const getPreference = useCallback(async ({ scope, key, callback }) => {
+    // const res = await localforage.getItem(key, callback);


What's going on with this commented-out line?

MikeTaylor · 2025-01-28T17:47:24Z

src/hooks/usePreferences.js

+    // so we use the `POST` endpoint for saving... 'PUT', and including the id in the path is used for updating
+    if (!id) {
+      try {
+        setId(prefId);


Would it make more sense to to move this line after the ky.post, so we only set the ID if the operation was successful?

MikeTaylor · 2025-01-28T17:48:04Z

src/hooks/usePreferences.js

+      try {
+        return ky.put(`settings/entries/${prefId}`, { json: payload });
+      } catch (err) {
+        console.log(`error updating preference at scope: ${scope}, and key: ${key} for user: ${userId} - ${err.message}`);


We can move this outside the try block to avoid duplicating it.

MikeTaylor · 2025-01-28T17:48:43Z

src/hooks/usePreferences.js

+    try {
+      if (id) {
+        await ky.delete(`settings/entries/${id}`);
+        setId(null);


I see we use ky.delete and then setId for deletion, as I think we should for creation.

sonarqubecloud · 2025-01-29T23:37:23Z

Quality Gate failed

Failed conditions
60.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

JohnC-80 added 5 commits January 27, 2025 09:57

add permissions for user preference management, add okapiInterface

b27ae44

add usePreferences hook

662db88

add AppOrderProvider context

92aa6e1

implement App Reordering, refactor MainNav to functional.

cab6928

Merge branch 'master' into STCOR-936

c36e6cf

zburke reviewed Jan 27, 2025

View reviewed changes

JohnC-80 added 4 commits January 28, 2025 10:26

recreat apps list on pathname change - fixes 'selectedApp'

c7bf306

Merge branch 'STCOR-936' of https://github.com/folio-org/stripes-core …

2ad7943

…into STCOR-936

AppOrderProvider: key query on user id

a78b938

AppOrderProvider - key query on user id

26b3037

MikeTaylor approved these changes Jan 28, 2025

View reviewed changes

JohnC-80 added 3 commits January 28, 2025 11:52

AppOrderProvider: clean comments

80f4a85

add tests for usePreferences hook

23e31bd

AppOrderProvider tests

0c6e189

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STCOR-936 - implement App-reordering, user preference management in stripes-core. #1584

STCOR-936 - implement App-reordering, user preference management in stripes-core. #1584

JohnC-80 commented Jan 27, 2025 •

edited by zburke

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

zburke Jan 27, 2025

MikeTaylor left a comment •

edited

Loading

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

zburke Jan 29, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

MikeTaylor Jan 28, 2025

sonarqubecloud bot commented Jan 29, 2025

STCOR-936 - implement App-reordering, user preference management in stripes-core. #1584

Are you sure you want to change the base?

STCOR-936 - implement App-reordering, user preference management in stripes-core. #1584

Conversation

JohnC-80 commented Jan 27, 2025 • edited by zburke Loading

Still Remaining:

github-actions bot commented Jan 27, 2025 • edited Loading

Bigtest Unit Test Results

github-actions bot commented Jan 27, 2025 • edited Loading

Jest Unit Test Results

Choose a reason for hiding this comment

MikeTaylor left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 29, 2025

Quality Gate failed

JohnC-80 commented Jan 27, 2025 •

edited by zburke

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

MikeTaylor left a comment •

edited

Loading