UIU-2943: ECS - Filter users by "User Type" #2555
Conversation
|
@alisher-epam as we can filter users by type, do not forget to revert #2543 |
|
@NikitaSedyx I will modify the changes and revert them manually in my current PR. |
alisher-epam
requested review from
Terala-Priyanka,
mariia-aloshyna,
Dmytro-Melnyshyn,
a team,
zburke and
NikitaSedyx
and removed request for
a team,
mariia-aloshyna,
Dmytro-Melnyshyn and
Terala-Priyanka
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
I got halfway through this but just gave up. Sorry. The PR description makes no attempt whatsoever to describe the purpose of this work, nor does it make any attempt to explain why the shadow-user details are being removed from the default query. The test descriptions do not match the test implementations. Additionally, the PR-template was left in place but not filled out.
I gave up somewhere in the UserSearchContainer tests.
Please clean this up and then re-request a review when you're actually ready for a review.
| import { screen } from '@folio/jest-config-stripes/testing-library/react'; | ||
|
|
||
| import renderWithRouter from 'helpers/renderWithRouter'; | ||
| import '../../../test/jest/__mock__/matchMedia.mock'; |
There was a problem hiding this comment.
import '__mock__/matchMedia.mock'
| it('should return false', () => { | ||
| const data = isConsortiumEnabled(); | ||
| expect(data).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Under what circumstances should it return false? This makes it sound like the function should always return false. Describe the situation you are actually testing. In this case, sth like "returns false (default) when given no data".
| it('should return true', () => { | ||
| const data = isConsortiumEnabled(STRIPES); | ||
| expect(data).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Under what circumstances should it return true? This makes it sound like it should always return true. Describe the situation you are testing, e.g. "returns true given a valid 'centralTenantId'". OOC, what happens/should happen when you pass a stripes object with different kinds of sparse data, e.g.
user.user.consortium === {}
user.user.consortium.centralTenantId === ''
user.user.consortium.centralTenantId === false
user.user.consortium.centralTenantId === null
Eh, never mind, this is somewhat covered in the tests below. The test descriptions could still be improved though.
src/views/UserSearch/Filters.js
Outdated
| import { isConsortiumEnabled } from '../../components/util'; | ||
| import { USER_TYPES, statusFilter } from '../../constants'; | ||
|
|
||
| const ACCORDION_ID_PREFIX = 'users_filter_accordion'; |
There was a problem hiding this comment.
Constants, ❤️ . But please keep the same value that was there previously, with hyphens instead of underscores. I know this didn't break any tests, but who knows what else might rely on that value; if we can preserve it then we should.
|
@zburke Thank you for helping and caring not only about the code base but also improving our tech skills. You are a really good mentor. God bless you! |
* UIU-2943: ECS - Filter users by "User Type" * tests: add tests * update: revert UIU-2933 manually * refactor: fix naming issues and minor improvements * tests: fix test labeling issue * tests: improve test cases
* UIU-2942: Assign/unassign a users affiliations adjustments * UIU-2943: ECS - Filter users by "User Type" (#2555) * UIU-2943: ECS - Filter users by "User Type" * tests: add tests * update: revert UIU-2933 manually * refactor: fix naming issues and minor improvements * tests: fix test labeling issue * tests: improve test cases * hide primary assigned affiliations * hide primary and central affiliations * tests: fix failing tests * tests: fix failing tests * display missing affiliations * hide central and primary affiliations * tests: fix failing tests * tests: fix failing tests * exclude primary affiliation * fix: wrong comparison affiliation ids * fix: not displaying affiliations * test: fix sematic testing issues
Purpose
UIU-2943
Approach
Screen.Recording.2023-09-13.at.12.41.59.AM.mov
TODOS and Open Questions
Learning
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
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.