Conversation
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
Leaving comment for PR reviewer
apps/frontend/src/containers/foodManufacturerDonationManagement.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/containers/foodManufacturerDonationManagement.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
| setCurrentPages(initialPages); | ||
| } catch (error) { | ||
| alert('Error fetching donations: ' + error); |
There was a problem hiding this comment.
might need to change depending on when justin's alert pr goes in
There was a problem hiding this comment.
will keep this comment unresolved till then.
amywng
left a comment
amywng
left a comment
There was a problem hiding this comment.
replied to some of the other comments as well
apps/frontend/src/containers/foodManufacturerDonationManagement.tsx
Outdated
Show resolved
Hide resolved
|
agree on moving recurrence logic to backend, makes sense to have the logic on frontend for rendering the designed next donation scheduled line (without calling backend) and on backend for actually setting the recurring donations. to add to the 56 comments! +1! |
amywng
left a comment
amywng
left a comment
There was a problem hiding this comment.
like lowk i feel bad but also i feel bad for myself... anyway keep up the great work!
| import { Test, TestingModule } from '@nestjs/testing'; | ||
| import { mock } from 'jest-mock-extended'; | ||
| import { Donation } from './donations.entity'; | ||
| import { FoodManufacturer } from '../foodManufacturers/manufacturers.entity'; |
There was a problem hiding this comment.
unnecessary import. also, need test for service propagating not found exception for findOne
| <Dialog.Body> | ||
| <Text mb="1.5em"> | ||
| Log a new donation by filling out the form below. | ||
| <Text mb={8} mt={-4} color="neutral.700"> |
There was a problem hiding this comment.
what do you think it should be? this seems to match the figma to me.
| padding={0} | ||
| > | ||
| <Box color="neutral.300"> | ||
| <Minus size={16} /> |
| }); | ||
| }); | ||
|
|
||
| describe('GET /:foodManufacturerId/donations', () => { |
There was a problem hiding this comment.
need test for service propagating not found exception
| foodRescue: boolean; | ||
| } | ||
|
|
||
| export type DayOfWeek = |
There was a problem hiding this comment.
can we move to this and RepeatOnState to types.ts so utils imports from there and not a component file
| import { RecurrenceEnum } from '../types'; | ||
| import { Type } from 'class-transformer'; | ||
|
|
||
| export class RepeatOnDaysDto { |
There was a problem hiding this comment.
any way we can check that at least one is true if the recurrence is weekly?
|
|
||
| const next = new Date(today); | ||
| // Date clamp back to 28 for monthly and yearly | ||
| if (next.getDate() > 28) next.setDate(28); |
There was a problem hiding this comment.
can we only do this when necessary (e.g. check if the month rolled over, and then set the day to the last day of the correct month)? if u think it's a good idea can u also implement in the backend function
| expect(timestamps).toEqual([...timestamps].sort((a, b) => a - b)); | ||
| }); | ||
|
|
||
| it('WEEKLY - does not include today even if today is selected', async () => { |
There was a problem hiding this comment.
maybe a better descriptor would be if today's DOW is selected?
4 participants
ℹ️ Issue
Closes #134
📝 Description
✔️ Verification
Took screenshots of the pages:
Verified each updated endpoint worked on Postman
🏕️ (Optional) Future Work / Notes
We will need to update this later on when we go to finish up the Action Required tab.