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-2966 - display item title and barcode as text for dcb virtual item. #2597

Merged
merged 5 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Disable open loan actions for virtual patron. Refs UIU-2964.
* Fix problem with Date field in User app reports does not populate when a first entry was cleared. Refs UIU-2991.
* Hide all actionalble buttons on user details pane for DCB Virtual user. Refs UIU-2987.
* Display item title and barcode as text when the item is dcb virtual item. Refs UIU-2966.

## [10.0.4](https://github.com/folio-org/ui-users/tree/v10.0.4) (2023-11-10)
[Full Changelog](https://github.com/folio-org/ui-users/compare/v10.0.3...v10.0.4)
Expand Down
4 changes: 4 additions & 0 deletions src/components/util/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
USER_TYPES,
requestStatuses,
sortTypes,
DCB_INSTANCE_ID,
DCB_HOLDINGS_RECORD_ID,
} from '../../constants';

/**
Expand Down Expand Up @@ -208,3 +210,5 @@ export const isStaffUser = (user) => user?.type === USER_TYPES.STAFF;
export const isAffiliationsEnabled = (user) => {
return !isPatronUser(user) && !isDcbUser(user);
};

export const isDCBItem = (item) => item.instanceId === DCB_INSTANCE_ID && item.holdingsRecordId === DCB_HOLDINGS_RECORD_ID;
3 changes: 3 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,3 +348,6 @@ export const USER_TYPES = {
SYSTEM: 'system',
DCB: 'dcb',
};

export const DCB_INSTANCE_ID = '9d1b77e4-f02e-4b7f-b296-3f2042ddac54';
export const DCB_HOLDINGS_RECORD_ID = '10cd3a5a-d36f-4c7a-bc4f-e1ae3cf820c9';
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad to see these values as constants, but to me they feel like the kind of thing that should be provided in an API request like /inventory/dcb-instance-id. I realize that's not on you, but it seems like bad policy to have a bunch of UUIDs copied around inside a bunch of different back-end and front-end modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, implementing such an API was not a part of architecture.

61 changes: 48 additions & 13 deletions src/views/LoanDetails/LoanDetails.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
accountsMatchStatus,
checkUserActive,
isDcbUser,
isDCBItem,
Copy link
Member

Choose a reason for hiding this comment

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

We need to decide between functions like isDCB... and isDcb.... I know that's not your gaffe, but it's highlighted here by adding the new import.

} from '../../components/util';
import { itemStatuses, loanActions, refundClaimReturned } from '../../constants';
import {
Expand Down Expand Up @@ -298,23 +299,56 @@ class LoanDetails extends React.Component {

showTitle(loan) {
this.loan = loan;
const title = `${get(this.loan, ['item', 'title'], '')}`;
const title = get(this.loan, ['item', 'title'], '');
const instanceId = get(this.loan, ['item', 'instanceId'], '');
const holdingsRecordId = get(this.loan, ['item', 'holdingsRecordId'], '');
const isVirtualItem = isDCBItem({ instanceId, holdingsRecordId });

if (title) {
const titleTodisplay = (title.length >= 77) ? `${title.substring(0, 77)}...` : title;
Copy link
Member

Choose a reason for hiding this comment

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

I know you didn't add this, but while you're here, can you move this magic number to a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented it. I hope this is the way you would suggest it to be.

return <KeyValue
const formattedValue = `${titleTodisplay} (${get(this.loan, ['item', 'materialType', 'name'])})`;
return (
<KeyValue
data-testId="item-title"
label={<FormattedMessage id="ui-users.loans.columns.title" />}
value={
isVirtualItem ?
`${formattedValue}` :
<Link to={`/inventory/view/${instanceId}`}>
{formattedValue}
</Link>
zburke marked this conversation as resolved.
Show resolved Hide resolved
}
/>
);
}

return (
<KeyValue
label={<FormattedMessage id="ui-users.loans.columns.title" />}
value={(
<Link to={`/inventory/view/${get(this.loan, ['item', 'instanceId'], '')}`}>
{`${titleTodisplay} (${get(this.loan, ['item', 'materialType', 'name'])})`}
</Link>
)}
/>;
value="-"
zburke marked this conversation as resolved.
Show resolved Hide resolved
/>
);
}

showBarcode(loan) {
this.loan = loan;
const instanceId = get(this.loan, ['item', 'instanceId'], '');
const holdingsRecordId = get(this.loan, ['item', 'holdingsRecordId'], '');
const itemId = get(this.loan, ['itemId'], '');
const itemBarcode = get(loan, ['item', 'barcode'], '');
const isVirtualItem = isDCBItem({ instanceId, holdingsRecordId });

if (isVirtualItem) {
return itemBarcode;
Copy link
Member

Choose a reason for hiding this comment

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

The diff makes it hard to assess, but do you need to handle items without barcodes? If so, set the default value of itemBarcode to <NoValue /> instead of empty string up on line 338.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the code that I have written now. It has been restructured by me.

  1. My primary question - is it possible for a loan be created on an item without barcode? I think "NO" in that case, in other words, there can be no loan whose item will not have a barcode. Is that ok to have the default value removed from line 338?
  2. The idea here is no render the item barcode as text (not a link) in case of a dcb item.

}

return <KeyValue
label={<FormattedMessage id="ui-users.loans.columns.title" />}
value="-"
/>;
return (
<Link
to={`/inventory/view/${instanceId}/${holdingsRecordId}/${itemId}`}
>
{itemBarcode}
</Link>
);
}

renderChangeDueDateDialog() {
Expand Down Expand Up @@ -627,8 +661,9 @@ class LoanDetails extends React.Component {
</Col>
<Col xs={2}>
<KeyValue
data-testId="item-barcode"
label={<FormattedMessage id="ui-users.loans.columns.barcode" />}
value={<Link to={`/inventory/view/${get(loan, ['item', 'instanceId'], '')}/${get(loan, ['item', 'holdingsRecordId'], '')}/${get(loan, ['itemId'], '')}`}>{get(loan, ['item', 'barcode'], '')}</Link>}
value={this.showBarcode(loan)}
/>
</Col>
<Col xs={2}>
Expand Down
40 changes: 39 additions & 1 deletion src/views/LoanDetails/LoanDetails.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import { screen } from '@folio/jest-config-stripes/testing-library/react';
import { screen, within } from '@folio/jest-config-stripes/testing-library/react';
import userEvent from '@folio/jest-config-stripes/testing-library/user-event';
// eslint-disable-next-line import/no-extraneous-dependencies
import okapiOpenLoan from 'fixtures/openLoan';
import okapiCurrentUser from 'fixtures/okapiCurrentUser';
import renderWithRouter from 'helpers/renderWithRouter';
import LoanDetails from './LoanDetails';
import {
DCB_INSTANCE_ID,
DCB_HOLDINGS_RECORD_ID,
} from '../../constants';

jest.useFakeTimers('legacy');
jest.mock('react-intl', () => ({
Expand Down Expand Up @@ -539,4 +543,38 @@ describe('LoanDetails', () => {
expect(screen.getByRole('button', { name:'ui-users.loans.newStaffInfo' })).toBeDisabled();
});
});

describe('when item is dcb item', () => {
const virtualItemPropsData = {
...propsData,
loan: {
...okapiOpenLoan,
item: {
...okapiOpenLoan.item,
instanceId: DCB_INSTANCE_ID,
holdingsRecordId: DCB_HOLDINGS_RECORD_ID,
title: 'Vitual item',
barcode: 'virtual barcode'
}
}
};

it('render item title not as a link but as text', () => {
renderLoanProxyDetails({
...virtualItemPropsData,
});

expect(screen.queryByRole('link', { name: /Virtual item/i })).toBeNull();
expect(within(screen.getByTestId('item-title')).queryByText('Virtual Item')).toBeDefined();
});

it('render item barcode as text', () => {
renderLoanProxyDetails({
...virtualItemPropsData,
});

expect(screen.queryByRole('link', { name: /Virtual barcode/i })).toBeNull();
expect(within(screen.getByTestId('item-barcode')).queryByText('Virtual barcode')).toBeDefined();
});
});
});
Loading