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

FOLIO-3253 POC tweak login to avoid users-bl #1101

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

zburke
Copy link
Member

@zburke zburke commented Aug 13, 2021

POC refactor of login to avoid users-bl, which has lots of
dependencies, in favor of authn/login, which has very few. This is not
intended to be pretty (obviously, the extra requests need cleaning up
and error handling) but just to demonstrate that it is possible to login
from the UI against a minimal set of back-end and front-end modules.

Refs FOLIO-3253

POC refactor of login to avoid `users-bl`, which has lots of
dependencies, in favor of `authn/login`, which has very few. This is not
intended to be pretty (obviously, the extra requests need cleaning up
and error handling) but just to demonstrate that it is possible to login
from the UI against a minimal set of back-end and front-end modules.

Refs FOLIO-3253
@zburke zburke marked this pull request as draft August 13, 2021 03:45
@github-actions
Copy link

github-actions bot commented Aug 13, 2021

Jest Unit Test Statistics

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit e572131. ± Comparison against base commit 17719ac.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 13, 2021

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   1m 18s ⏱️ + 1m 3s
275 tests +4  240 ✔️  - 28  1 💤 ±0  34 +32 
274 runs  ±0  239 ✔️  - 32  1 💤 ±0  34 +32 

For more details on these failures, see this check.

Results for commit e572131. ± Comparison against base commit 17719ac.

This pull request removes 5 and adds 7 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_100_0_4896_60_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
Chrome_100_0_4896_60_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
Chrome_100_0_4896_60_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_100_0_4896_60_(Linux_x86_64).Forgot username form test ‑ Forgot username form 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_100_0_4896_60_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation
Chrome_100_0_4896_60_(Linux_x86_64).useCustomFields hook ‑ useCustomFields hook requests for existing custom fields "after each" hook for "should have custom fields"
Chrome_100_0_4896_60_(Linux_x86_64).useCustomFields hook ‑ useCustomFields hook requests for non-existing custom fields "after each" hook for "should have error"
Chrome_100_0_4896_60_(Linux_x86_64).useCustomFields hook ‑ useCustomFields hook requests for version-compatible custom fields "after each" hook for "should have custom fields"
Chrome_100_0_4896_60_(Linux_x86_64).useCustomFields hook ‑ useCustomFields hook requests for version-incompatible custom fields "after each" hook for "should have error"

♻️ This comment has been updated with latest results.

@zburke zburke changed the title FOLIO-3253 tweak login to avoid users-bl FOLIO-3253 POC tweak login to avoid users-bl Feb 8, 2022
@MikeTaylor
Copy link
Contributor

@zburke This work falls into the category of "never urgent, always important". What does it take — either organizationally or technically — to get this happen? Now that I am once more running a "minimal" FOLIO backend in a VM on my desktop, and find myself allocation a ludicrous 20 Gb of memory to it, it's both ethically and practically intolerable that things should continue as they are.

@MikeTaylor
Copy link
Contributor

Pinging @wafschneider so he gets notified of further comments.

@MikeTaylor
Copy link
Contributor

@skomorokh and I are keen for this to get merged. @zburke, what hinders us?

@zburke
Copy link
Member Author

zburke commented Aug 29, 2022

@MikeTaylor, @skomorokh: In talking with a few other folks including @skoczko and @mkuklis, our thinking around the best approach to FOLIO-3253 has shifted. This PR involves authenticating directly against mod-login (which has very few dependencies) but that endpoint just returns a token, nothing else. Thus, a side-effect is the need to then federate calls to (new) /_self endpoints in mod-users, mod-permissions, and mod-optional-thingies in order to hydrate the stripes user object.

An alternative is to leverage the fact that mod-users-bl already handles mod-inventory (from when service points) as an optional dependency and could handle other mod-optional-thingies in the same way. That eliminates new backend work (no new /_self endpoints) and leaves the frontend much the same:

  1. in stripes-core, stash the JSON response from the login request in the redux store (STCOR-649)
  2. let other apps respond to the LOGIN event and pull addReducer from context to update the stripes object (UISP-32)
  3. remove service-point handling from stripes-core (a breaking change because setServicePoints and setCurServicePoint are exported, STCOR-650)

I am actively working on (1) and (2).

@MikeTaylor
Copy link
Contributor

That all makes sense -- thank you!

@mkuklis
Copy link
Contributor

mkuklis commented Aug 29, 2022

Related to STCOR-650 this is the place that will have to be refactored:

https://github.com/folio-org/ui-users/blob/56c938ffab7948da3c9a3084155d9a27db4ddb84/src/components/Wrappers/withServicePoints.js#L5-L11

We will have to think about where to put it. It could live in ui-servicepoints handler and instead of coupling it with the ui-users we could trigger an event via HandlerManager.

@MikeTaylor
Copy link
Contributor

That sounds right to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants