UIPFU-77 - Add 'User assignment status' filter group #249

Terala-Priyanka · 2024-01-10T07:53:24Z

Purpose

UIPFU-77 - Add "User assignment status" filter in find-user-plugin

Approach

The requirement is to create a new filter group in this plugin.

This filter group should be conditionally rendered in this plugin i.e., ATM from user settings permission set detail only.
This filter group is implemented differently than any regular filter group.
Existing functionality remains same.

How is it different?
The new filters do not participate in cql formation.
The cql that could be formed by this filter group could not be supported by BE API for various other challenges and design reasons. Hence an alternative mechanism is chosen to populate the users based on "Assigned" and "Unassinged" filters.

assigned and unassigned filter strings are omitted to include in cql formation.
when assigned filter is selected alone or in combination with other filter groups, no cql will is formed and no API call is made, instead list of users is rendered based on initialSelectedUsers.
when unassigned filter is selected, filter string is altered by removing 'unassigned' filter string and add others based on the scenario in context. The results of fetch users api is filtered not to include initial selected users.

Unit tests

We will gradually migrate unit tests in this module to Jest/RTL .
For the new files, Jest minimal setup has been introduced and unit tests have been written.

Screencast

DFcQyHnddK.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.

github-actions · 2024-01-10T07:53:56Z

Jest Unit Test Statistics

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

Results for commit 4bd8c31. ± Comparison against base commit 1e3f510.

♻️ This comment has been updated with latest results.

github-actions · 2024-01-10T07:54:13Z

BigTest Unit Test Statistics

17 tests ±0 17 ✔️ ±0 0s ⏱️ ±0s
  1 suites ±0   0 💤 ±0
  1 files ±0   0 ❌ ±0

Results for commit 4bd8c31. ± Comparison against base commit 1e3f510.

♻️ This comment has been updated with latest results.

BogdanDenis · 2024-01-10T13:52:06Z

src/UserSearchContainer.js

@@ -32,6 +34,28 @@ const compileQuery = template(
  { interpolate: /%{([\s\S]+?)}/g }
 );

+export function buildQuery(queryParams, pathComponents, resourceData, logger, props) {
+  const filters = props.initialSelectedUsers ? filterConfigWithUserAssignedStatus : filterConfig;
+  const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.substring(`${UAS}`) ? updateResourceData(resourceData) : resourceData;


I'm not sure about the usage of substring here. Is this method from String? If so, then it only accepts startIndex and endIndex

BogdanDenis · 2024-01-10T14:37:09Z

src/utils.js

+    * When UnAssigned filter is selected incombination with any other filters(in other filter groups),
+    * filter astring for Unassigned is removed and th erest of the filter string is propagated to makeQueryFunction.
+    */
+    const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(`${UAS}`)).join(',');


Won't startsWith(`${UAS}`) remove uas.Assigned too? In case when there are filters selected from other groups and both Assigned and Unassigned are checked?

uas.Assigned should be removed in any case, as it will never participate in cql formation. So here the logic is appropriate. I see a need to update the comment above this code of line. Thank you.

BogdanDenis · 2024-01-10T14:38:08Z

src/utils.js

+    /*
+    * When Assigned filter is selected on 'User assigbment Status' filter group, in any combination of filters in other filter groups,
+    * cql formation is not needed.
+    * hence remove aus filter before propagating it further to makeQueryFunction


Probably needs to be uas instead of aus

BogdanDenis · 2024-01-10T14:38:41Z

src/utils.js

+  } else if (filterString.includes(`${UNASSIGNED_FILTER_KEY}`)) {
+    /*
+    * When UnAssigned filter is selected incombination with any other filters(in other filter groups),
+    * filter astring for Unassigned is removed and th erest of the filter string is propagated to makeQueryFunction.


A few typos here

BogdanDenis · 2024-01-10T14:40:07Z

src/utils.js

+    newRData.query.filters = alteredfilters;
+  } else if (filterString.includes(`${ASSIGNED_FILTER_KEY}`)) {
+    /*
+    * When Assigned filter is selected on 'User assigbment Status' filter group, in any combination of filters in other filter groups,


I don't understand this case. If there 100 assigned users, and we need to see only staff members for example - can we not do this?

For any combination (from other filter groups) with "Assigned" filter, there is no need to fetch the users once again(as the users list is already available through initialSelectedUsers prop) and hence we don't need any filters to participate in cql formation.
getUsersBasedOnAssignmentStatus does the needful.

BogdanDenis · 2024-01-10T14:41:23Z

src/utils.js

+};
+
+const filterUsersList = (filterString, initialSelectedUsers, users, filterCheck) => {
+  let usersList;


Let's make it an empty array by default to prevent any exceptions

BogdanDenis · 2024-01-10T14:42:38Z

src/utils.js

+  let usersList;
+  if (filterString === `${ASSIGNED}`) {
+    const assignedUsers = Object.values(initialSelectedUsers);
+    if (filterCheck) {


Can give filterCheck argument a default value () => true to remove this if else

filterCheck argument is not always needed. Function overloading is used here.

BogdanDenis · 2024-01-10T14:43:07Z

src/utils.js

+  } else if (filterString === `${UNASSIGNED}`) {
+    // when ONLY "Unassigned" filter is selected
+    const assignedUserIds = Object.keys(initialSelectedUsers);
+    if (filterCheck) {


Same here regarding filterCheck

src/utils.js

+  return usersList;
+};
+
+// eslint-disable-next-line consistent-return


src/utils.test.js

+      const activeFilterState = { uas: ['Assigned'] };
+      const uasFilterValue = ['Assigned'];
+      const initialSelectedUsers = {
+        '7daa365a-d8c1-4e5d-90ac-ab38f8230827': {


src/utils.js

+
+// eslint-disable-next-line consistent-return
+export const getUsersBasedOnAssignmentStatus = (activeFilterState, uasFilterValue, initialSelectedUsers, users) => {
+  const condForOneOfTheFilters = (u) => activeFilterState?.active?.includes(u.active ? `${ACTIVE}` : `${INACTIVE}`) || activeFilterState?.pg?.includes(u.patronGroup);


BogdanDenis · 2024-01-11T13:40:46Z

src/UserSearchContainer.js

+      }
+      const uasFilterValue = activeFilters.split(',').filter(f => f.includes(`${UAS}`))[0].split('.')[1];
+
+      let otherFilterGroups = activeFilters.split(',').filter(f => !f.includes(`${UAS}`)).map(f => f.split('.')[0]);


We don't need check other filter values because this result list is alredy returned from back-end for a search with these filters. We just need to select from fetched users assigned or not assigned based on uas filter value

if (activeFilters.includes(`${UAS}`)) { const assignedUserIds = Object.keys(initialSelectedUsers); const uasFilterValues = activeFilters.split(',').filter(f => f.find(`${UAS}`)); // both or neither Assignment status values are selected, so we can return result from BE if (uasFilterValues.length === 2 || uasFilterValues.length === 0) { return fetchedUsers; } if (uasFilterValues.includes(ASSIGNED_FILTER_KEY)) { return fetchedUsers.filter(user => assignedUserIds.includes(user.id)); } else { return fetchedUsers.filter(user => !assignedUserIds.includes(user.id)); } } return fetchedUsers;

Thank you again for your review!
I agree that it is not needed for Unassigned filter as we are propagating the filters to make cql. But I think it is needed in case of Assigned filter as we are not propagating the filter string to make the cql(as we don't actually need to fetch)

artem-blazhko

Could you cover new functionality in UserSearchContainer.js file by jest/RTL tests?

artem-blazhko · 2024-01-12T11:36:30Z

src/UserSearchContainer.js

@@ -32,6 +39,28 @@ const compileQuery = template(
  { interpolate: /%{([\s\S]+?)}/g }
 );

+export function buildQuery(queryParams, pathComponents, resourceData, logger, props) {
+  const filters = props.initialSelectedUsers ? filterConfigWithUserAssignedStatus : filterConfig;
+  const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.includes(`${UAS}`) ? updateResourceData(resourceData) : resourceData;


Suggested change

const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.includes(`${UAS}`) ? updateResourceData(resourceData) : resourceData;

const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.includes(UAS) ? updateResourceData(resourceData) : resourceData;

artem-blazhko · 2024-01-12T11:36:52Z

src/UserSearchContainer.js

+    const activeFilters = get(resources, 'query.filters', '');
+    const assignedUsers = Object.values(initialSelectedUsers);
+
+    if (activeFilters === `${ASSIGNED_FILTER_KEY}`) return assignedUsers;


Suggested change

if (activeFilters === `${ASSIGNED_FILTER_KEY}`) return assignedUsers;

if (activeFilters === ASSIGNED_FILTER_KEY) return assignedUsers;

artem-blazhko · 2024-01-12T11:37:30Z

src/UserSearchContainer.js

+
+    if (activeFilters === `${ASSIGNED_FILTER_KEY}`) return assignedUsers;
+
+    if (activeFilters.includes(`${UAS}`)) {


Suggested change

if (activeFilters.includes(`${UAS}`)) {

if (activeFilters.includes(UAS)) {

artem-blazhko · 2024-01-12T11:37:55Z

src/UserSearchContainer.js

+
+    if (activeFilters.includes(`${UAS}`)) {
+      const assignedUserIds = Object.keys(initialSelectedUsers);
+      const hasBothUASFilters = activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);


Suggested change

const hasBothUASFilters = activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);

const hasBothUASFilters = activeFilters.includes(ASSIGNED_FILTER_KEY) && activeFilters.includes(UNASSIGNED_FILTER_KEY);

artem-blazhko · 2024-01-12T11:38:39Z

src/UserSearchContainer.js

+      const hasNoneOfUASFilters = !activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && !activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);
+      const uasFilterValue = activeFilters.split(',').filter(f => f.includes(`${UAS}`))[0].split('.')[1];


Suggested change

const hasNoneOfUASFilters = !activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && !activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);

const uasFilterValue = activeFilters.split(',').filter(f => f.includes(`${UAS}`))[0].split('.')[1];

const hasNoneOfUASFilters = !activeFilters.includes(ASSIGNED_FILTER_KEY) && !activeFilters.includes(UNASSIGNED_FILTER_KEY);

const uasFilterValue = activeFilters.split(',').filter(f => f.includes(UAS))[0].split('.')[1];

artem-blazhko · 2024-01-12T11:39:35Z

src/UserSearchContainer.js

+        return fetchedUsers;
+      }
+
+      if (uasFilterValue === `${ASSIGNED}`) {


Suggested change

if (uasFilterValue === `${ASSIGNED}`) {

if (uasFilterValue === ASSIGNED) {

artem-blazhko · 2024-01-12T11:41:47Z

src/UserSearchContainer.js

@@ -168,6 +181,35 @@ class UserSearchContainer extends React.Component {
    return get(this.props.resources, 'query', {});
  }

+  getUsers = () => {


I would suggest to move this function outside the component. I will much easier to test all if...else conditions.
But it is up to you.

artem-blazhko · 2024-01-12T11:44:16Z

src/utils.js

+export const updateResourceData = (rData) => {
+  const filterString = rData?.query?.filters;
+  const newRData = cloneDeep(rData);
+  if (filterString === `${UNASSIGNED_FILTER_KEY}` || filterString === `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}` || filterString === `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`) {


Suggested change

if (filterString === `${UNASSIGNED_FILTER_KEY}` || filterString === `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}` || filterString === `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`) {

if (filterString === UNASSIGNED_FILTER_KEY || filterString === `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}` || filterString === `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`) {

artem-blazhko · 2024-01-12T11:44:51Z

src/utils.js

+    const alteredfilters = 'active.active,active.inactive';
+    newRData.query.filters = alteredfilters;
+  } else {
+    const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(`${UAS}`)).join(',');


Suggested change

const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(`${UAS}`)).join(',');

const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(UAS)).join(',');

artem-blazhko · 2024-01-12T11:45:35Z

src/utils.test.js

+
+describe('updatedResourceData', () => {
+  describe('when only UnAssigned filter is selected', () => {
+    [`${UNASSIGNED_FILTER_KEY}`, `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}`, `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`].forEach(filterStr => (


Suggested change

[`${UNASSIGNED_FILTER_KEY}`, `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}`, `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`].forEach(filterStr => (

[UNASSIGNED_FILTER_KEY, `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}`, `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`].forEach(filterStr => (

Terala-Priyanka · 2024-01-12T12:21:13Z

Could you cover new functionality in UserSearchContainer.js file by jest/RTL tests?

Hey @artem-blazhko
We will soon start working on migrating to Jest/RTL. So this file will be covered in the scope of that ticket that is there in our backlog.

src/UserSearchContainer.js

Co-authored-by: Artem Blazhko <[email protected]>

sonarqubecloud · 2024-01-12T13:51:05Z

Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

UIPFU-77 - Add 'User assignment status' filter group

0024421

Terala-Priyanka marked this pull request as draft January 10, 2024 07:53

Terala-Priyanka added 6 commits January 10, 2024 13:24

UIPFU-77 - Update change log

b2d41d4

UIPFU-77 - yarn.lock

5b390ff

UIPFU-77 - update test script

a309cc4

UIPFU-77 - refine complexity

e5287a2

UIPFU-77 - cleanup

a7bba68

UIPFU-77 - add comments

5ad35b6

Terala-Priyanka requested a review from a team January 10, 2024 12:57

BogdanDenis requested changes Jan 10, 2024

View reviewed changes

BogdanDenis requested a review from a team January 10, 2024 15:27

BogdanDenis requested changes Jan 10, 2024

View reviewed changes

Terala-Priyanka marked this pull request as ready for review January 11, 2024 06:47

UIPFU-77 - fix review comments

a016d97

Terala-Priyanka requested review from BogdanDenis and a team January 11, 2024 12:16

BogdanDenis requested changes Jan 11, 2024

View reviewed changes

UIPFU-77 - fix review comments

a3adb92

Terala-Priyanka requested review from BogdanDenis and a team January 11, 2024 16:04

artem-blazhko requested changes Jan 12, 2024

View reviewed changes

artem-blazhko requested a review from a team January 12, 2024 11:48

UIPFU-fix review comments

16e11ee

Terala-Priyanka requested review from artem-blazhko and zburke January 12, 2024 12:40

artem-blazhko reviewed Jan 12, 2024

View reviewed changes

src/UserSearchContainer.js Outdated Show resolved Hide resolved

Update src/UserSearchContainer.js

4bd8c31

Co-authored-by: Artem Blazhko <[email protected]>

Terala-Priyanka requested a review from artem-blazhko January 12, 2024 13:49

artem-blazhko approved these changes Jan 12, 2024

View reviewed changes

BogdanDenis approved these changes Jan 15, 2024

View reviewed changes

Terala-Priyanka merged commit 32f0730 into master Jan 16, 2024
5 of 6 checks passed

Terala-Priyanka deleted the UIPFU-77 branch January 16, 2024 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIPFU-77 - Add 'User assignment status' filter group #249

UIPFU-77 - Add 'User assignment status' filter group #249

Terala-Priyanka commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

BogdanDenis Jan 10, 2024

BogdanDenis Jan 10, 2024

Terala-Priyanka Jan 11, 2024

BogdanDenis Jan 10, 2024

BogdanDenis Jan 10, 2024

BogdanDenis Jan 10, 2024

Terala-Priyanka Jan 11, 2024 •

edited

Loading

BogdanDenis Jan 10, 2024

BogdanDenis Jan 10, 2024

Terala-Priyanka Jan 11, 2024

BogdanDenis Jan 10, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

BogdanDenis Jan 11, 2024

Terala-Priyanka Jan 11, 2024

artem-blazhko left a comment

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

artem-blazhko Jan 12, 2024

Terala-Priyanka commented Jan 12, 2024

sonarqubecloud bot commented Jan 12, 2024

	const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.includes(`${UAS}`) ? updateResourceData(resourceData) : resourceData;
	const updatedResourceData = props.initialSelectedUsers && resourceData?.query?.filters?.includes(UAS) ? updateResourceData(resourceData) : resourceData;

	if (activeFilters === `${ASSIGNED_FILTER_KEY}`) return assignedUsers;
	if (activeFilters === ASSIGNED_FILTER_KEY) return assignedUsers;


		if (activeFilters === `${ASSIGNED_FILTER_KEY}`) return assignedUsers;

		if (activeFilters.includes(`${UAS}`)) {

	if (activeFilters.includes(`${UAS}`)) {
	if (activeFilters.includes(UAS)) {

	const hasBothUASFilters = activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);
	const hasBothUASFilters = activeFilters.includes(ASSIGNED_FILTER_KEY) && activeFilters.includes(UNASSIGNED_FILTER_KEY);

		const hasNoneOfUASFilters = !activeFilters.includes(`${ASSIGNED_FILTER_KEY}`) && !activeFilters.includes(`${UNASSIGNED_FILTER_KEY}`);
		const uasFilterValue = activeFilters.split(',').filter(f => f.includes(`${UAS}`))[0].split('.')[1];

	if (uasFilterValue === `${ASSIGNED}`) {
	if (uasFilterValue === ASSIGNED) {

	if (filterString === `${UNASSIGNED_FILTER_KEY}` \|\| filterString === `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}` \|\| filterString === `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`) {
	if (filterString === UNASSIGNED_FILTER_KEY \|\| filterString === `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}` \|\| filterString === `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`) {

	const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(`${UAS}`)).join(',');
	const alteredfilters = newRData.query.filters.split(',').filter(str => !str.startsWith(UAS)).join(',');

	[`${UNASSIGNED_FILTER_KEY}`, `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}`, `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`].forEach(filterStr => (
	[UNASSIGNED_FILTER_KEY, `${ASSIGNED_FILTER_KEY},${UNASSIGNED_FILTER_KEY}`, `${UNASSIGNED_FILTER_KEY},${ASSIGNED_FILTER_KEY}`].forEach(filterStr => (

UIPFU-77 - Add 'User assignment status' filter group #249

UIPFU-77 - Add 'User assignment status' filter group #249

Conversation

Terala-Priyanka commented Jan 10, 2024 • edited Loading

Purpose

Approach

Unit tests

Screencast

Pre-Merge Checklist

github-actions bot commented Jan 10, 2024 • edited Loading

Jest Unit Test Statistics

github-actions bot commented Jan 10, 2024 • edited Loading

BigTest Unit Test Statistics

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Terala-Priyanka Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-blazhko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Terala-Priyanka commented Jan 12, 2024

sonarqubecloud bot commented Jan 12, 2024

Quality Gate failed

Terala-Priyanka commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

github-actions bot commented Jan 10, 2024 •

edited

Loading

Terala-Priyanka Jan 11, 2024 •

edited

Loading