UISACQCOMP-166: view list of donors #724
Conversation
| export const useFetchDonors = () => { | ||
| const ky = useOkapiKy(); | ||
|
|
||
| const { isLoading, mutateAsync } = useMutation( |
There was a problem hiding this comment.
Why don't you use useQuery for fetching by ids?
There was a problem hiding this comment.
The mutation will be passed as a prop inside the other components and it will call the mutation on the demand.
There was a problem hiding this comment.
Yeah, but you have the query key to invalidate the fn and you can use ids of donors for that purpose. I know, that there are examples of using useMutation to fetch the data, but probably useQuery will bring more benefits, like caching.
lib/DonorsList/DonorsList.js
Outdated
|
|
||
| import { acqRowFormatter } from '../utils'; | ||
| import AddDonorButton from './AddDonorButton'; | ||
| import { alignRowProps, columnMapping, columnWidths, visibleColumns } from './constants'; |
There was a problem hiding this comment.
Please, break imports into separate lines
There was a problem hiding this comment.
How could I miss this step again =(
lib/DonorsList/DonorsList.js
Outdated
| }) => ({ | ||
| name: donor => donor.name, | ||
| code: donor => donor.code, | ||
| unassignDonor: (donor) => ( |
There was a problem hiding this comment.
Please keep consistency in code style: two args in the callbacks before not wrapped in the (..)
lib/DonorsList/DonorsList.js
Outdated
| align="end" | ||
| aria-label={intl.formatMessage({ id: 'stripes-acq-components.donors.button.unassign' })} | ||
| buttonStyle="fieldControl" | ||
| data-test-unassign-donor |
There was a problem hiding this comment.
Do you use this attr. anywhere?
| export const useFetchDonors = () => { | ||
| const ky = useOkapiKy(); | ||
|
|
||
| const { isLoading, mutateAsync } = useMutation( |
There was a problem hiding this comment.
Yeah, but you have the query key to invalidate the fn and you can use ids of donors for that purpose. I know, that there are examples of using useMutation to fetch the data, but probably useQuery will bring more benefits, like caching.
lib/DonorsList/DonorsContainer.js
Outdated
|
|
||
| useEffect(() => { | ||
| if (donorOrganizationIds.length) { | ||
| handleFetchDonors(donorOrganizationIds); |
There was a problem hiding this comment.
It seems you don't need extra function handleFetchDonors to update ids now
There was a problem hiding this comment.
Agree, sorry the implementation was for mutation. I will remove it.
There was a problem hiding this comment.
Actually, I meant handleFetchDonors callback: it is redundant, because it does the same as a setDonorIds, so you can use it directly, and the useEffect was fine 🙂
lib/DonorsList/DonorsList.js
Outdated
| _index, | ||
| }; | ||
| }); | ||
| const contentData = sortBy(donors, [({ lastName }) => lastName?.toLowerCase()]); |
There was a problem hiding this comment.
probably makes sense to wrap in useMemo, because sorting is quite an expensive operation
| ); | ||
|
|
||
| return ({ | ||
| donors: data || [], |
There was a problem hiding this comment.
could you please create a constant for default data (like DEFAULT_DATA) outside the hook to always return the same reference on each rerender.
alisher-epam
requested review from
usavkov-epam
and removed request for
BogdanDenis
lib/DonorsList/DonorsContainer.js
Outdated
| name={name} | ||
| id={name} | ||
| component={DonorsList} | ||
| fetchDonors={handleFetchDonors} |
There was a problem hiding this comment.
should be called like setDonorIds and accept setDonorIds, because now the callback does not fetch anything
lib/DonorsList/AddDonorButton.js
Outdated
| const newDonorsIds = map(donors.filter(({ id }) => !addedDonorIds.has(id)), 'id'); | ||
|
|
||
| if (newDonorsIds.length) { | ||
| fetchDonors([...addedDonorIds, ...newDonorsIds]); |
There was a problem hiding this comment.
the fetchDonors should be called smth like onAddDonor, btn doesn't fetch anything
| .json() | ||
| .then(({ organizations }) => organizations), | ||
| donorOrganizationIds, | ||
| buildQueryByIds, |
There was a problem hiding this comment.
redundant as batchRequest build query by provided ids by default
lib/DonorsList/DonorsContainer.js
Outdated
| import { useFetchDonors } from './hooks'; | ||
|
|
||
| function DonorsContainer({ name, donorOrganizationIds }) { | ||
| const [donorIds, setDonorIds] = useState(() => donorOrganizationIds); |
There was a problem hiding this comment.
Why is lazy initialization needed here?
There was a problem hiding this comment.
Good point. We can remove it. Initial implementation I had a function to get the ids.
| visibleFilters, | ||
| } from './constants'; | ||
|
|
||
| const AddDonorButton = ({ onAddDonors, fields, stripes, name }) => { |
There was a problem hiding this comment.
Please add a test file for this
|
Kudos, SonarCloud Quality Gate passed! |
Purpose
UISACQCOMP-166 - View the list of donors associated with a given record
so that accurately track donors and donations
Approach
Screen.Recording.2023-10-31.at.1.49.35.PM.mov
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.