-
Notifications
You must be signed in to change notification settings - Fork 3
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
UINV-470: Invoices - Show in version history record view, which fields have been edited #827
Conversation
…s have been edited
Jest Unit Test Statistics 1 files ±0 122 suites +4 3m 36s ⏱️ +13s Results for commit b57257f. ± Comparison against base commit d68e810. This pull request removes 3 and adds 9 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
return ky.get(CONFIG_API, { searchParams }).json(); | ||
}; | ||
|
||
export const getVersionMetadata = (version, entity) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks as a reusable tool for all modules, that will use version history approach. It makes sense to keep it in acq-components lib
MODULE_TENANT, | ||
} from '@folio/stripes-acq-components'; | ||
|
||
export const getTenantAddresses = (ky) => async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a organization version's specific request to be placed here. Moreover there is a useAddresses
in acq lib, probably it should satisfy fetching of addresses in useSelectedInvoiceVersion
. If you need exacly a function instead of a hook, I suggest keeping it in acq lib as well here https://github.com/folio-org/stripes-acq-components/tree/master/lib/utils/api
addressesMap, | ||
] = await Promise.all([ | ||
fetchOrganizationsByIds(ky)(organizationIds).then(({ organizations }) => keyBy(organizations, 'id')), | ||
fetchExportDataByIds({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are the same code in another module https://github.com/folio-org/ui-orders/blob/master/src/common/utils/getAcqUnitsByIds.js
It's an indicator to keep it in the shared place (ACQ lib). Could be implemented consistently with existing methodshttps://github.com/folio-org/stripes-acq-components/tree/master/lib/utils/api
api: ACQUISITIONS_UNITS_API, | ||
records: 'acquisitionsUnits', | ||
}).then(data => keyBy(data, 'id')), | ||
getTenantAddresses(ky)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same here, duplication should be reduced since version history will spread in other modules
Quality Gate passedIssues Measures |
Purpose
UINV-470: Invoices - Show in version history record view, which fields have been edited
Approach
Screen.Recording.2024-11-19.at.20.36.53.mp4
Screenshots
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.