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-689 STCOR-690 Support switch consortium active affiliation #1308

Merged
merged 6 commits into from
May 10, 2023

Conversation

NikitaSedyx
Copy link
Contributor

@NikitaSedyx NikitaSedyx commented May 8, 2023

Purpose

https://issues.folio.org/browse/STCOR-689
https://issues.folio.org/browse/STCOR-690

As a central tenant of a consortium, it should be able to switch the current context to manage institutional tenants. Header X-Okapi-Tenant acts as a pointer to the current context when sending requests to the server.


If a user login into the consortium central tenant, he expects to see its name in the top-right corner of the screen (in the profile dropdown trigger, above a current SP name).

Approach

  • Validate user based on current tenant value stored in a session (if absent - use tenant from stripes configs);
  • Expose updateTenant service, which will update the current tenant, fetch resources (perms, SPs and etc.) and update session;
  • Allow <HandlerManager> to invoke handlers for other module types (app, settings);
  • Render active tenant (affiliation) name in the profile dropdown trigger;
  • Implement unit test;
Preview

firefox_Wtyh0AQ9Zx

@NikitaSedyx NikitaSedyx self-assigned this May 8, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

Jest Unit Test Statistics

62 tests  +6   62 ✔️ +6   17s ⏱️ +9s
  9 suites +1     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 15a26af. ± Comparison against base commit 7c3e75b.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 8, 2023

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   11s ⏱️ ±0s
271 tests ±0  266 ✔️ ±0  5 💤 ±0  0 ±0 
274 runs  ±0  269 ✔️ ±0  5 💤 ±0  0 ±0 

Results for commit 15a26af. ± Comparison against base commit 7c3e75b.

♻️ This comment has been updated with latest results.

@NikitaSedyx
Copy link
Contributor Author

hi @mkuklis @zburke please review one more implementation for consortia stuff (follows recommendation to keep it agnostic to compare with #1307)

@@ -20,7 +20,15 @@ class HandlerManager extends React.Component {
constructor(props) {
super(props);
const { event, stripes, modules, data } = props;
this.components = getEventHandlers(event, stripes, modules.handler, data);
const uniqueModules = Object.values(modules)
Copy link
Contributor

Choose a reason for hiding this comment

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

@NikitaSedyx I think the original idea for this was to only support the handler modules. Could you explain why was this changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because non-handler modules can't handle events like LOGIN, according to https://github.com/folio-org/stripes/blob/master/doc/dev-guide.md#handlers-and-events

Stripes modules of any type can provide a handler; the handler module type is provided for the case where the handler is the only thing the module provides, i.e. there is no main-screen app and no settings.

and it's strange that we provide a handler in app or settings module, but it can't handle events

Copy link
Contributor

Choose a reason for hiding this comment

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

I think each existing ui module can be also registered as a handler (in stripes config) and also introduce the actual static handler. Is that not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as an alternative, we can add handler type to our module, but it will contradict with description of handler module, additionally it will require to refactor ProfileDropdown logic to handle duplicates (actually the same logic that is applied here)

IMO following current documentation, HandlerManager should support handler of all module types, not just handlers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a question of interpretation, both ways work for me :)

Copy link
Contributor Author

@NikitaSedyx NikitaSedyx May 8, 2023

Choose a reason for hiding this comment

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

so we have two use cases to use handlers: one doesn't require module as a handler, another one requires

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zburke @mkuklis reverted changes for HandlerManager, probably handler docs section should be updated, at least 2 devs were confused by it :)

Copy link
Contributor

@mkuklis mkuklis May 8, 2023

Choose a reason for hiding this comment

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

Thank you @NikitaSedyx. I agree with you that the docs should include the information about how to turn the existing ui-module into a handler and that the HandlerManager is intended to be used only by the hander modules. Do you think that would be sufficient?

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 believe it should be enough, I was a bit confused by 2 points

  1. any module can provide handler (but in fact ignored by HandlerManager)
  2. handler type is used for case when module provides only handler

looks like what you mentioned covers them

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @NikitaSedyx

@mkuklis mkuklis self-requested a review May 8, 2023 19:28
const servicePointName = userData?.curServicePoint?.name;
const tenantName = userData?.tenants?.find(({ id }) => id === okapi.tenant)?.name;

const withLabel = Boolean(servicePointName || tenantName);
Copy link
Member

Choose a reason for hiding this comment

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

A better convention than with... is is... or has.... We use withFoo as a naming convention for HOCs that provide a Foo object to the component they wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, missed this migrating from #1307 PR

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

88.2% 88.2% Coverage
0.0% 0.0% Duplication

@NikitaSedyx NikitaSedyx merged commit 15d2ee7 into master May 10, 2023
4 of 5 checks passed
@NikitaSedyx NikitaSedyx deleted the STCOR-690 branch May 10, 2023 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants