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

UITEN-278 - create reading room from tenant settings #393

Merged
merged 18 commits into from
Apr 29, 2024

Conversation

Terala-Priyanka
Copy link
Contributor

@Terala-Priyanka Terala-Priyanka commented Apr 23, 2024

Purpose

https://folio-org.atlassian.net/browse/UITEN-278 - Reading Room Access (settings): Create new reading room

Note

The base branch for this PR is not master. Volaris will merge this feature branch for ui-tenant-settings module at a little later point in time. Also, base branch is always maintained equivalent to master.

Screencast

chrome_iC8SY6pYmJ.mp4

Pre-Merge Checklist

Before merging this PR, please go through the following list and take appropriate actions.

  • I've added appropriate record to the CHANGELOG.md
  • Does this PR meet or exceed the expected quality standards?
    • Code coverage on new code is 80% or greater
    • Duplications on new code is 3% or less
    • There are no major code smells or security issues
  • Does this introduce breaking changes?
    • If any API-related changes - okapi interfaces and permissions are reviewed/changed correspondingly
    • There are no breaking changes in this PR.

If there are breaking changes, please STOP and consider the following:

  • What other modules will these changes impact?
  • Do JIRAs exist to update the impacted modules?
    • If not, please create them
    • Do they contain the appropriate level of detail? Which endpoints/schemas changed, etc.
    • Do they have all they appropriate links to blocked/related issues?
  • Are the JIRAs under active development?
    • If not, contact the project's PO and make sure they're aware of the urgency.
  • Do PRs exist for these changes?
    • If so, have they been approved?

Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.

While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.

Copy link

github-actions bot commented Apr 23, 2024

Jest Unit Test Statistics

    1 files  ±0  28 suites  ±0   2m 17s ⏱️ +4s
100 tests +3  88 ✔️ +3  12 💤 ±0  0 ±0 
102 runs  +3  90 ✔️ +3  12 💤 ±0  0 ±0 

Results for commit e623a35. ± Comparison against base commit 8db6552.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Apr 23, 2024

BigTest Unit Test Statistics

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

Results for commit e623a35. ± Comparison against base commit 8db6552.

♻️ This comment has been updated with latest results.

@Terala-Priyanka Terala-Priyanka changed the base branch from master to RRA-Settings April 23, 2024 07:08
@Terala-Priyanka Terala-Priyanka changed the base branch from RRA-Settings to master April 23, 2024 07:09
@Terala-Priyanka Terala-Priyanka marked this pull request as draft April 23, 2024 07:10
@Terala-Priyanka Terala-Priyanka changed the base branch from master to RRA-Settings April 23, 2024 07:22
@Terala-Priyanka Terala-Priyanka marked this pull request as ready for review April 24, 2024 03:24
@Terala-Priyanka Terala-Priyanka requested review from a team, manvendra-s-rathore and gurleenkaurbp and removed request for a team, gurleenkaurbp and manvendra-s-rathore April 24, 2024 03:24
@Terala-Priyanka Terala-Priyanka marked this pull request as draft April 24, 2024 03:29
@Terala-Priyanka Terala-Priyanka marked this pull request as ready for review April 26, 2024 04:15
});
});

const options = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please use reduce, I know it's the same but as for me looks more clear

const options = servicePoints.reduce((acc, s) => {
  if (!sps.includes(s.id) || s.name === 'None') {
    acc.push({ value: s.id, label: s.name });
  }
  return acc;
}, []);

* A reading room can have more than one service points assigned to it.
* but a servicepoint cannot be mapped to more than one reading room
*/
const sps = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

What about to handle it with base array methods to avoid this tracking of closures?

Maybe like this:

const sps = rrs.reduce((acc, rr) => {
  acc.push(...(rr.servicePoints || []).map((s) => s.value));

  return [...new Set(acc)];
}, []);

Or maybe

const sps = rrs
  .map((rr) => rr.servicePoints || [])
  .map((s) => s.value)
  .flat();

const uniqSps = [...new Set(spss)];

Note: I didn't check it live with your code, just as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quickly check, but it seems not equivalent. I prefer to retain my implementation.

import { readingRoomAccessColumns } from './constant';

const validators = {
[readingRoomAccessColumns.NAME]: function validateName(item) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there is any reason use name function instead of just arrow? Looks like it calls by key and this name is visible only 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.

updated to anonymous arrow functions

}, []);

const validate = (item, index, items) => {
const itemErrors = validateItem(item, items) || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Items errors looks like redundant, could you please just change to
return validateItem(item, items) || {}

Copy link

sonarcloud bot commented Apr 29, 2024

@Terala-Priyanka Terala-Priyanka requested a review from vashjs April 29, 2024 04:54
@Terala-Priyanka Terala-Priyanka merged commit 75343f0 into RRA-Settings Apr 29, 2024
6 checks passed
@Terala-Priyanka Terala-Priyanka deleted the UITEN-278 branch April 29, 2024 09:39
Terala-Priyanka added a commit that referenced this pull request May 2, 2024
* UITEN-276 - Reading Room Access settings page basic layout implementation (#389)

* UITEN-276 - Reading Room Access settings page basic layout implementation

* UITEN-276 - fix review comments

* UITEN-278 - create reading room from tenant settings (#393)

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-278 - fix review comments

* UIU-282, UIU-283 - Implementation for Update and Delete of Reading Room Access (#398)

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-282, UITEN-283 - Implementation for update and delete reading room access.
Terala-Priyanka added a commit that referenced this pull request May 3, 2024
…ent value (follow up) (#400)

* UITEN-276 - Reading Room Access settings page basic layout implementation (#389)

* UITEN-276 - Reading Room Access settings page basic layout implementation

* UITEN-276 - fix review comments

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-278 - create reading room from tenant settings (#393)

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-278 - fix review comments

* UITEN-282, UITEN-283 - Implementation for update and delete reading room access.

* UITEN-282 - fix the default value of Public checkbox to pick the current value
Terala-Priyanka added a commit that referenced this pull request May 7, 2024
…ow-up) (#401)

* UITEN-276 - Reading Room Access settings page basic layout implementation (#389)

* UITEN-276 - Reading Room Access settings page basic layout implementation

* UITEN-276 - fix review comments

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-278 - create reading room from tenant settings (#393)

* UITEN-278 - create reading room from tenant settings

* UITEN-278 - update package.json

* UITEN-278 - Add prop type validation

* UITEN-278 - update jest config

* UITEN-278 - add unit test

* UITEN-278 - update to final form

* UITEN-278 - update uni test

* UITEN-278 - refinement

* UITEN-278 - add validators, refine code

* UITEN-278 - refine

* UITEN-278 - disable lint rule

* UITEN-278 - refine

* UITEN-278 - fix prop types sonar problem

* UITEN-278 - update permission check on reading room access

* UITEN-278 - fix review comments

* UITEN-282, UITEN-283 - Implementation for update and delete reading room access.

* UITEN-282 - fix the default value of Public checkbox to pick the current value

* UITEN-282 - add null checks for isPublic field value and also have a default value in case it is not available.

* UITEN-282 - refine code

* UITEN-282 - refine the validation for service points

* UITEN-282 - refine the validation for service points
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