-
Notifications
You must be signed in to change notification settings - Fork 1
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
Job data model change #66
Job data model change #66
Conversation
* s/income/job * s/payments/payment * import payment details from the right file
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.
fyi-- prereviewed per Kate's request. No comments; lgtm.
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.
This file is net new and is the Form
for adding and editing expenses. This is a side effect of the expenses work here
@@ -16,7 +16,7 @@ describe('Add Income To Ledger Page', async () => { | |||
})) | |||
mockRouter.push('/job/expense/add') | |||
store = makeStore() | |||
render (<Provider store={store}><Page /></Provider>) | |||
render (<Provider store={store}><Page params={{idx: '0'}} /></Provider>) |
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.
this adds the job param. that's what idx
is here
@@ -59,7 +59,7 @@ describe('Add Income To Ledger Page', async () => { | |||
|
|||
await waitFor(() => { | |||
expect(mockRouter).toMatchObject({ | |||
asPath: "/job/expense/list" | |||
asPath: "/job/list" |
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.
this is the flow change choice i made to keep it moving
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.
this looks new, but it's actually moving the expense directory under the job/[idx]
dir and then ripping out the <Form>
to separate concerns
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.
Thanks for separating concerns.
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.
this is also a file move that looks net new but it's not
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.
this file was moved to the one below it in this list
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.
all of [jobSlice
, expensesSlice
, and paymentSlice
] and their tests should look very similar. they are all using the same ideas and should be refactored to reuse logic eventually (i will make a ticket for it)
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.
this is stub page until we know what data we'll want to add
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.
stub (see next file)
*/ | ||
|
||
export const createUuid = () => uuidv4() |
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.
i'm not 100% sure this is the right place for this method, but here it will live until i find a better spot
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.
I appreciate the step by step comments-- super helpful.
control={control} | ||
rules={{ required: {value:true, message: t('add_expense_date_required')} }} | ||
render={({ field }) => ( | ||
<> |
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.
This empty <>
is making my brain glitch. What does <></>
do? Div?
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.
i'm pretty sure it's going to render the root element
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.
Thanks for separating concerns.
About this pull request
Ticket: FFS-1775
Main goal: Move job, expense, and payment data model and methods to reflect a more relational state as discussed in redux documentation.
New data model structure
This will be the structure for
payments
andexpenses
as well.payments
andexpenses
will have an index back to their associatedjob
to allow for streamlined data storage and relational behavior.What's in here
Data-model specific
Side effects
expense/add
form into its own page likejob/add
+IncomeFormJob
andpayment/add
+IncomeFormPayment
expenses
pages underjob
in the routing/file structure to pass the correct paramsstatement/
section to get passing tests and created FFS-1818 for that work