[SSF-213] Create new donation fixes#179
Conversation
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
I'm noticing that the FloatingAlert component uses a status of info | error, whereas the AlertState interface uses success | error. We should standardize this (in my opinion, info | error everywhere). Can we change all the success alert messages to be info instead, create an enum in types to be reused in both the FloatingAlert and AlertState, make it so that all FloatingAlerts now just have status={alertState.status}?
| (file) => file.size > MAX_FILE_SIZE, | ||
| ); | ||
| if (oversized) { | ||
| setErrorMessage( |
There was a problem hiding this comment.
Can we make this an alert message as well?
| const navigate = useNavigate(); | ||
| const [application, setApplication] = useState<FoodManufacturer | null>(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const [error, setError] = useState<{ |
There was a problem hiding this comment.
There are many other files that use this [error, setError] useState. Can we get rid of all of this and wrap these errors in the message as well?
|
|
||
| mockFoodManufacturersService.findByUserId.mockResolvedValueOnce({ | ||
| foodManufacturerId: 1, | ||
| } as any); |
There was a problem hiding this comment.
Can we type this to a FoodManufacturer instead? Thats what it normally returns
| @Body() body: CreateDonationDto, | ||
| ): Promise<Donation> { | ||
| return this.donationService.create(body); | ||
| const manufacturer = await this.foodManufacturersService.findByUserId( |
There was a problem hiding this comment.
If you look at other instances of using req.user.id, we normally fetch the user within the service. For consistency purposes, can we move the logic into the service, and adjust the tests accordingly to organize it there?
| onOpenChange={(e: { open: boolean }) => { | ||
| if (!e.open) { | ||
| setAlertMessage(''); | ||
| setAlertMessage('', 'error'); |
| } | ||
| setErrorMessage(''); | ||
| setSuccessMessage(''); | ||
| setAlertMessage('', 'error'); |
jxuistrying
left a comment
jxuistrying
left a comment
There was a problem hiding this comment.
When I was smoke testing front end for FM users I noticed both dashboard and all pages are currently highlighted at the same time is that intended?
3 participants
ℹ️ Issue
Closes SSF-213
📝 Description
Backend:
POST /api/donationsendpoint so only Food Manufacturers can access itFrontend:
alerthook to include astatusfieldalerthookapps/frontend/src/containers/foodManufacturerDonationManagement.tsx✔️ Verification
createDonationendpointSucess:
Error:
🏕️ (Optional) Future Work / Notes
Edit: updated the backend test and now all tests pass
manufacturerIDis still necessary inapps/frontend/src/containers/donationManagement.tsx