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-905: based on 'users-keycloak' interface use bl-users or users-keycloak for _self endpoint in useUserTenantPermissions #1556

Merged
merged 12 commits into from
Nov 15, 2024

Conversation

aidynoJ
Copy link
Contributor

@aidynoJ aidynoJ commented Nov 4, 2024

Purpose
STCOR-905 - based on 'users-keycloak' interface use bl-users or users-keycloak for _self endpoint in useUserTenantPermissions #1556

Approach
Remove redundant hooks and make changes in useUserTenantPermissions

@aidynoJ aidynoJ requested review from zburke and ryandberger November 4, 2024 09:48
Copy link

github-actions bot commented Nov 4, 2024

Bigtest Unit Test Results

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

Results for commit c9fb762. ± Comparison against base commit 921f0ab.

This pull request removes 194 and adds 192 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_130_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_130_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_130_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_130_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_130_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_130_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_130_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_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_131_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_131_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_131_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_130_0_0_0_(Linux_x86_64).PasswordValidationField with an invalid password ‑ PasswordValidationField with an invalid password shows a validation error
Chrome_130_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_130_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_130_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_130_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_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

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 4, 2024

Jest Unit Test Results

  1 files  ±0   55 suites   - 2   1m 31s ⏱️ -1s
348 tests  - 3  348 ✅  - 3  0 💤 ±0  0 ❌ ±0 
351 runs   - 3  351 ✅  - 3  0 💤 ±0  0 ❌ ±0 

Results for commit c9fb762. ± Comparison against base commit 921f0ab.

This pull request removes 4 and adds 1 tests. Note that renamed tests count towards both.
useUserSelfTenantPermissions should fetch user permissions for specified tenant ‑ useUserSelfTenantPermissions should fetch user permissions for specified tenant
useUserTenantPermissionNames should fetch user permissions for specified tenant ‑ useUserTenantPermissionNames should fetch user permissions for specified tenant
useUserTenantPermissions should return _self permissions data when "roles" interface is present ‑ useUserTenantPermissions should return _self permissions data when "roles" interface is present
useUserTenantPermissions should return tenant permissions data when "roles" interface is NOT present ‑ useUserTenantPermissions should return tenant permissions data when "roles" interface is NOT present
useUserTenantPermissions should fetch user permissions for specified tenant ‑ useUserTenantPermissions should fetch user permissions for specified tenant

♻️ This comment has been updated with latest results.

@aidynoJ aidynoJ marked this pull request as draft November 6, 2024 13:09
@ryandberger
Copy link
Member

Is this still a draft PR?

@aidynoJ
Copy link
Contributor Author

aidynoJ commented Nov 7, 2024

Is this still a draft PR?

Just wanted to change implementation of useUserTenantPermissions.

@aidynoJ aidynoJ marked this pull request as ready for review November 7, 2024 12:55
@aidynoJ aidynoJ changed the title STCOR-905: Add userId parameter to useUserTenantPermissions hook. STCOR-905: based on 'users-keycloak' interface use bl-users or users-keycloak for _self endpoint in useUserSelfTenantPermissions Nov 13, 2024
@aidynoJ aidynoJ requested a review from ryandberger November 13, 2024 13:12
@ryandberger
Copy link
Member

Please fix lint issue.

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.

Code changes look correct to me, but changing the hook's name is a breaking change. If the target is sunflower, that's OK (we know that release will contain breaking changes), but we should do that work in two phases:

  1. in stripes-core, export this hook by its old and new names
  2. in other repositories, refactor to the new name
  3. in stripes-core, remove the export of the old name

That avoids the need to merge PRs here and across the UI repositories at the same moment. If we go that route, how about a shorter name, e.g. useTenantPermissions? Permissions don't exist outside the context of a user, so that part of the name feels redundant.

Alternatively, just leave the name as-is. That allows consumers of this hook (inventory, marc-authorities, quick-marc, and stripes-authority-components) to stay as-is, and ui-users can name its local hook whatever it wants to, even if it overlaps; because it isn't importing from stripes-core there isn't a naming conflict, even if it's confusing.

Also, lint :)

@aidynoJ
Copy link
Contributor Author

aidynoJ commented Nov 14, 2024

Code changes look correct to me, but changing the hook's name is a breaking change. If the target is sunflower, that's OK (we know that release will contain breaking changes), but we should do that work in two phases:

  1. in stripes-core, export this hook by its old and new names
  2. in other repositories, refactor to the new name
  3. in stripes-core, remove the export of the old name

That avoids the need to merge PRs here and across the UI repositories at the same moment. If we go that route, how about a shorter name, e.g. useTenantPermissions? Permissions don't exist outside the context of a user, so that part of the name feels redundant.

Alternatively, just leave the name as-is. That allows consumers of this hook (inventory, marc-authorities, quick-marc, and stripes-authority-components) to stay as-is, and ui-users can name its local hook whatever it wants to, even if it overlaps; because it isn't importing from stripes-core there isn't a naming conflict, even if it's confusing.

Also, lint :)

Let's choose the easiest way, leave it as is and go further. Developers can decide how to call their local hooks whatever they want :)

@aidynoJ aidynoJ requested a review from zburke November 14, 2024 12:34
@aidynoJ aidynoJ changed the title STCOR-905: based on 'users-keycloak' interface use bl-users or users-keycloak for _self endpoint in useUserSelfTenantPermissions STCOR-905: based on 'users-keycloak' interface use bl-users or users-keycloak for _self endpoint in useUserTenantPermissions Nov 15, 2024
@aidynoJ aidynoJ merged commit 943d67a into master Nov 15, 2024
16 checks passed
@aidynoJ aidynoJ deleted the STCOR-905 branch November 15, 2024 09:01
zburke pushed a commit that referenced this pull request Dec 2, 2024
…keycloak for _self endpoint in useUserTenantPermissions (#1556)

Refs STCOR-905.

(cherry picked from commit 943d67a)
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.

3 participants