UIU-2964 - Disable open loan actions for virtual patron #2588
Conversation
| return 'Lost and paid'; | ||
| describe('LoanDetails', () => { | ||
| describe('Render LoanProxyDetails component', () => { | ||
| it('When props ID and proxy ID are same with Loan Missing', () => { |
There was a problem hiding this comment.
There are a lot of tests that existed before this PR that don't follow the describe/it structure. These it statements don't tell what the expected behavior is.
I understand that it's out of scope of the ticket to refactor tests completely, so could you just fix the structure a bit? Wrap it statements in describe. describe should have the pre-conditions explained and it should have the expected behaviour?
There was a problem hiding this comment.
I see there are many tests that are not properly structured.
I think it will be good to address them in separate ticket, in order to keep the purpose of this PR in the scope of the ticket. Is that fine with you?
There was a problem hiding this comment.
Sure, please create a ticket then and it can be worked on separately
There was a problem hiding this comment.
Hey @BogdanDenis
Created a ticket for the same - https://issues.folio.org/browse/UIU-3001
Please review and adjust any details in you wish to. Thank you
src/constants.js
Outdated
| @@ -348,3 +348,7 @@ export const USER_TYPES = { | |||
| SYSTEM: 'system', | |||
| DCB: 'dcb', | |||
| }; | |||
|
|
|||
| export const DCB_USER = { | |||
| lastName: 'DcbSystem' | |||
There was a problem hiding this comment.
Can we use this function to identify DCB users, instead of a hard-coded last name?
https://github.com/folio-org/ui-users/blob/master/src/components/util/util.js#L205
If not, maybe by a constant id if one exists? I'm just thinking that it might be possible to create multiple users with this last name
There was a problem hiding this comment.
Thank you! I have refined the logic accordingly. Also, cleaned up the new files.
| loanIsMissing: false, | ||
| userEvent.click(getAllByText('ui-users.loans.declareLost')[0]); | ||
| userEvent.click(getAllByText('ui-users.loans.declareLost')[1]); | ||
| userEvent.click(screen.queryAllByRole('button')[0]); |
There was a problem hiding this comment.
I guess it's better to use one approach to be consistent here - either use screen.someQuery or distract all queries from your render component method
There was a problem hiding this comment.
@mariia-aloshyna
It is an existing code. It can be addressed in the scope of https://issues.folio.org/browse/UIU-3001
I have added a reference to your comment in the ticket description.
|
Kudos, SonarCloud Quality Gate passed! |
Purpose
UIU-2964 - [DCB] Loan details: Disable loan actions (Lending library)
Approach
TODOS and Open Questions
Screencast
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.