add lending to event creating/editing - #5863
Conversation
b9ce526 to
82306b4
Compare
82306b4 to
22ad34b
Compare
jonasdeluna
left a comment
There was a problem hiding this comment.
This is very cool! Could we have some relation between the event and requests, so that we can see/filter on the event page or in the ending page based on events? The event could for example store its own lending requests?
Could be a separate more extensive feature though
8513cb5 to
f554c29
Compare
bce2305 to
b7ab5ef
Compare
…ty fetch, clear stale availability, add tests
b20825b to
aee42ac
Compare
aee42ac to
5b793bc
Compare
ch0rizo
left a comment
There was a problem hiding this comment.
I think this a nice and useful feature!! There are some bugs and fixes I would take a closer look.
| for (let i = 0; i < values.lendingObjects.length; i++) { | ||
| const lendingRequestData: CreateLendingRequest = { | ||
| lendableObject: values.lendingObjects[i].value, | ||
| comment: values.lendingDescription[i] || '', |
There was a problem hiding this comment.
| comment: values.lendingDescription[i] || '', | |
| comment: values.lendingDescription?[i] || '', |
Crash when an object is selected but no comment typed. Here you fix it with a default empty string. However, lendingDescription is not in the initialValues. If the user selects a lending object, but never types into any comment field, values.lendingDescription is undefined -> underfined[i] throws on submit. Basically since values.lendingDescription is an array it will fail before hitting the default ''
| void dispatch(setSaveForUse(key, token, true)); | ||
| } | ||
|
|
||
| // Create lending request if lendingObjects exist and we're creating a new event |
There was a problem hiding this comment.
I am worried that this will create a lending request on editing the event aswell. Because there are no difference between edit and create form. There should gated with isEditPage.
| export const fetchAvailableLendableObjectIdsByDate = ( | ||
| start_date: string | Dateish, | ||
| end_date: string | Dateish, | ||
| ) => | ||
| callAPI<EntityId[]>({ | ||
| types: LendableObjects.FETCH_AVAILABLE, | ||
| endpoint: '/lending/objects/available/', | ||
| query: { start_date, end_date }, | ||
| meta: { | ||
| errorMessage: 'Henting av tilgjengelige utlånsobjekter feilet', | ||
| }, | ||
| propagateError: true, | ||
| }); |
There was a problem hiding this comment.
Availability fetch keys off startTime/endTime which is never change in the form (LendingSection.tsx). The form's date field is name="date" (a range) -> values.date[0]/values.date[1]. There is no date-driven startTime/endTime on form values. This means:
- On create: startTime/endtime are undefined -> always hits the clear branch, availability is never fetched.
- On edit: they hold the stale initial values -> fetched once with the original dates, never updates when the user picks a new range.
However onSubmit correctly uses values.date[0]/values.date[1] for the request - so the two halves of the feature disagree. A possible solution is to switch the availability effect to values.date?.[0] / values.date?.[1]
| // eslint-disable-next-line no-console | ||
| console.error('fetchAllLendableObjects failed', err); |
There was a problem hiding this comment.
I would not leave this in prod. If errors is to be announced it should be through user friendly toasts
| // eslint-disable-next-line no-console | ||
| console.error('fetchAvailableLendableObjectIdsByDate failed', err); |
| // Attach a requestId to meta for request identity (used to ignore stale responses) | ||
| const requestId = `${Date.now()}-${Math.random().toString(36).slice(2, 9)}`; |
There was a problem hiding this comment.
This is a global change. callAPI.ts is a the foundation of every redux API action in the app. This will add requestId to every action's meta.
Maybe reconsider making a global change for a local need. A proposed solution would be to solve the stale-response handling locally in LendingSection, without touching callAPI at all. The thunk from dispatch(fetchAvailable…) already returns a promise - you can await it and compare against the latest dispatched date range (or keep the "latest request" marker in a useRef (probably not a ideal🤷♂️)).
| FETCH_AVAILABLE: generateStatuses('LendableObject.FETCH_AVAILABLE'), | ||
| FETCH_AVAILABILITY: generateStatuses('LendableObject.FETCH_AVAILABILITY'), |
There was a problem hiding this comment.
What is the difference? Should there be a single source of truth, if they have the same purpose
There was a problem hiding this comment.
Its a bit confusing with the naming convention. Seems one is for bulk availability and the other one for a specific one
| state.lastAvailabilityRequestId = action.meta?.requestId ?? null; | ||
| }, | ||
| ); | ||
| addCase( | ||
| LendableObjects.FETCH_AVAILABLE.SUCCESS, | ||
| (state, action: AnyAction) => { | ||
| // Only apply success payload if it matches the latest request id | ||
| const requestId = action.meta?.requestId ?? null; | ||
| if (requestId && state.lastAvailabilityRequestId !== requestId) { | ||
| // stale response — ignore | ||
| return; | ||
| } | ||
| state.availabilityFetchFailed = false; | ||
| state.availableIds = action.payload; | ||
| }, | ||
| ); | ||
| addCase( | ||
| LendableObjects.FETCH_AVAILABLE.FAILURE, | ||
| (state, action: AnyAction) => { | ||
| const requestId = action.meta?.requestId ?? null; |
There was a problem hiding this comment.
Consider rewriting this if you didn't change callAPI.ts
There was a problem hiding this comment.
Just curious what is this for?
Description
Add the possibility of lending an object directly from the event edit form
Result
If you've made visual changes, please check the boxes below and include images showing the changes. Descriptions are appreciated.
Caution
Make sure your images do not contain any real user information.
Testing
Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.
Resolves #5862