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

UIU-2729/UIU-2730: Number generator usage for user barcode #2231

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

EthanFreestone
Copy link

@EthanFreestone EthanFreestone commented Sep 15, 2022

feat: number generator

Adds the number generator functionality to User barcode.
Also setting page to enable/disable this function, protected by a permission: ui-users.settings.numberGenerator.manage

refs UXPROD-2142, UIU-2729, UIU-2730

Added a setting "use number generator for user barcode", which disables the user input field "Barcode" on user create. Offered instead is a button to select from a list of Number Generator Sequences under the Generator "Patron", set up in the ui-service-interaction settings page.

Linting
Added 3 options, useGenerator/useTextField/useBoth under settings, and utilised those in the Form itself
Added test for settings page (Untested so far as I can't run the tests locally rn)
@EthanFreestone EthanFreestone changed the title feat: number generator feat: number generator, refs UXPROD-2142 Sep 15, 2022
@github-actions
Copy link

github-actions bot commented Sep 15, 2022

Jest Unit Test Statistics

0 files   -     1  0 suites   - 174   0s ⏱️ - 4m 52s
0 tests  - 743  0 ✔️  - 739  0 💤  - 4  0 ±0 
0 runs   - 766  0 ✔️  - 762  0 💤  - 4  0 ±0 

Results for commit 80993f8. ± Comparison against base commit 958870a.

♻️ This comment has been updated with latest results.

@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

56.5% 56.5% Coverage
0.0% 0.0% Duplication

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.

@folio/service-integration (whoops, @folio/service-interaction; my bad) is an application, not a library. Apps don't depend on apps, so, sorry but I reject this out of hand. Maybe NumberGeneratorModalButton should be moved into stripes-components or stripes-util, and then we can open a ui-users PR.

Additionally, adding @folio/service-integration as a dependency effectively makes mod-service-interaction a dependency of ui-users, which I don't think we want. Given the requirement stated in UXPROD-2442 is merely "barcode number does need to conform to any pattern, just generate random unique numbers" and we already have a uniqueness validator on the add/edit user form, this feels like overkill when you could just add a button with a callback like

const generateBarcode = (max, min) => Math.random() * (max - min) + min;

@JohnC-80, @mkuklis, penny for your thoughts?

A few other points of order:

  • Was @folio/service-integration ever submitted to the Tech Council for review? Including it on the reference build is not the same as having passed technical review. The deadline for acceptance of new modules by TC is September 30 and they ask for up to three weeks for review.
  • Please use yarn to manipulate package.json. Computers are better at alphabetizing things than people are 😆 . Yeah, yeah, pedantic, but letting the tools do their jobs works out better in the long run and results in smaller PRs later (because the whole file doesn't get rewritten/reorganized).
  • This PR needs to be attached to a UIU Jira ticket, not just a UXPROD. A PR in a given repository must have a Jira in the corresponding project in order to be able to align the technical work with a release, i.e. to be able to track that barcode-generation was added in Nolana because UIU-999 has its release field set to Nolana.
  • PRs titles should start with the Jira slug, e.g. "UIU-123 provide barcode number generator" in order to be able to quickly reference Jira when reviewing commit history and to assist release scripts that can pluck it from the commit subject and assign the fix-version in Jira automatically.

@mkuklis
Copy link
Contributor

mkuklis commented Sep 15, 2022

I agree with @zburke here. I actually can't find the source code for @folio/service-integration. It seems like overkill to bring it as a dependency if the only thing we need is a button.

@mkuklis
Copy link
Contributor

mkuklis commented Sep 15, 2022

Oh I see that this button is doing more: https://github.com/folio-org/ui-service-interaction/blob/master/src/public/components/NumberGeneratorButton/NumberGeneratorButton.js

And you are also thinking differently about the settings module by exporting some parts of it to the public:

https://github.com/folio-org/ui-service-interaction/tree/master/src/public

This aligns well with the micro frontend approach where each module can also expose some parts of itself to the public.

Perhaps we should discuss it a bit more.

@EthanFreestone EthanFreestone marked this pull request as draft September 16, 2022 09:19
Added optional Okapi interface dependency on servint and made setting page render conditionally on that interface, as well as only displaying
@EthanFreestone
Copy link
Author

Thank you @zburke and @mkuklis, have moved PR to draft while we discuss.

mod-service-interaction is designed to be a module which offers optional functionality to whichever clients wish to consume them. One of these is the number generation. As per your notes here I have added servint as an optional okapi interface dep and protected the code in ui-users with hasInterface.

As for the app vs library, we can separate out the offered components into their own repository for sure, however all ui-service-interaction offers is a configuration settings page, and that's all it should EVER offer. Still happy to move it out, but it feels icky to need to separate out the configuration piece from the offering of components using that config. (I am aware that this is how stripes be though, so we should probably make that move).
MKuklis is right here with our thinking:

And you are also thinking differently about the settings module by exporting some parts of it to the public:
https://github.com/folio-org/ui-service-interaction/tree/master/src/public
This aligns well with the micro frontend approach where each module can also expose some parts of itself to the public.
Perhaps we should discuss it a bit more.

On the TC inclusion criteria:

Was @folio/service-integration ever submitted to the Tech Council for review? Including it on the reference build is not the same as having passed technical review. The deadline for acceptance of new modules by TC is September 30 and they ask for up to three weeks for review.

It was not, but to be fair we also didn't ask for this to go into the reference builds... which ended up causing the dashboard failure a few weeks ago. This PR represents the first actual use case for this work, and I don't know if we have a need for this to go into Nolana (@ianibo can comment on that)

This PR needs to be attached to a UIU Jira ticket, not just a UXPROD. A PR in a given repository must have a Jira in the corresponding project in order to be able to align the technical work with a release, i.e. to be able to track that barcode-generation was added in Nolana because UIU-999 has its release field set to Nolana.

This is an oversight on our part, I'm sure we can get a proper ui-users ticket added.

Let yarn do the deps to avoid big switchups later down the line
@EthanFreestone EthanFreestone changed the title feat: number generator, refs UXPROD-2142 UIU-2729/UIU-2730: Number generator usage for user barcode Jan 12, 2023
Tweaks after pulling from latest master, edited translations and fixed small bug with ConfigManager usage.

refs UIU-2729, UIU-2730
Update generator code to use the new codes defined in this PR:

folio-org/mod-service-interaction#72
Update to use servint 3
Added some translations and brought button UX in line with other implementations
Fixes to tests
Changed Button from fullWidth to scale with text
Ensure num gen perm is visible
Accidentally left console logs in place
Accidentally committed local change
@EthanFreestone EthanFreestone marked this pull request as ready for review March 1, 2024 11:39
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