Skip to content
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

Edit and delete for [expenses|payments] #76

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

kategreenUSDS
Copy link
Collaborator

Ticket Number

FFS-1487
FFS-1688 (i think the payments edit ticket in sprint 8.3)

Description of changes

This work is to enable edit and delete for both payment and expense. Payment is net new, so there was a few more things to add there. Expenses had delete before, but not edit. Also I needed to move expenses to support the job/[jobId]/expense/[expenseId]/edit routing structure to call up the right item.

List of changes

  • move expense edit page to the right file+routing structure
  • add payment edit page
  • add date functions to test fixture generation + some tests
  • add tests for edit functionality
  • edit summary screens (/job/list) and (/job/[jobId]/expenses/list) -- which I know isn't congruent, routing-wise, but I held on this till design is closer to done

Requests for reviewer

Follow on work

  • Form validation and fixing how the datepicker is populated will be follow on work in ticket TODO Kate

Screenshots and demonstration

The best way to handle this is to run it locally.

@@ -1,13 +1,15 @@
'use client'

import ErrorSummary from "@/app/components/ErrorSummary"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is reordering of imports to be more standard (react->get data->render pages)

@@ -23,12 +25,34 @@ export type FormPaymentData = {
export default function FormPayment(params: FormPaymentProps) {
const { t } = useTranslation()

let formatDate = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a world where the date methods i wrote for the tests could be generalized as utils. however, it's important to have some separation of the code under test and the code used to test. i've left these separate for that reason, but this could be a change down the line. the deciding question there will be "by combining date utils, am i still testing what i think i'm testing?" truss, maintainer of React USWDS disagrees with me

@@ -52,12 +76,16 @@ export default function FormPayment(params: FormPaymentProps) {
name="date"
control={control}
rules={{ required: {value:true, message: t('add_income_required_field')} }}
render={({ field }) => (
render={({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a buncha churn on getting the datepicker and form validation working. i left them in as a demonstration at how it could work. they don't affect functionality (or, what didn't work before still doesn't work now lolsob)

@@ -8,12 +8,12 @@ import { Button, Form, FormGroup, Checkbox, DatePicker, ComboBox, Label, Require
import { Controller, SubmitHandler, useForm } from "react-hook-form"
import { useTranslation } from "react-i18next"

interface ExpenseFormPaymentProps {
onSubmit: SubmitHandler<ExpenseFormPaymentData>
interface FormExpenseProps {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming consistency change


it('should load all existing items ot the page', () => {
expect(screen.getByTestId('amount')).toHaveProperty('value', expense1.item.amount.toString())
// expect(screen.getByTestId('date-picker-external-input')).toHaveProperty('value', expense1.item.date)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all items that use the form validation <Control> element are commented out here since they don't work as expected and the tests fail. i left them commented out because once this work is underway, you can uncomment these, TDD-style and get moving from my base assumptions on how they should wrok


it('should load all existing items to the page', () => {
expect(screen.getByTestId('amount')).toHaveProperty('value', payment1.item.amount.toString())
// expect(screen.getByTestId('date-picker-external-input')).toHaveProperty('value', payment1.item.date)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as the expense data + the broken <Control> element/datepicker

import Page from './page'
import { EnhancedStore } from '@reduxjs/toolkit/react'
import { makeStore } from '@/lib/store'

import { addJob } from '@/lib/features/job/jobSlice'

/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to the test date generator

@@ -28,7 +28,6 @@ describe('List Income in Ledger Page', async () => {

it('shows navigation buttons', () => {
render (<Provider store={store}><Page /></Provider>)
expect(screen.getByTestId('add_another_button')).toBeDefined()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the design change from the last PR, this button is superfluous

const router = useRouter()

const routeToStandardDeductionElection = useAppSelector(state => recommendStandardDeduction(state))
const { t } = useTranslation()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to rework how the job list page + its stubs IncomeFormList and IncomeFormListItem were iterating to support mulitple jobs + mulitple payments

describe('Medicaid and Snap', () => {
let benefits: BenefitsState
beforeEach(() => {
benefits = generateBenefits({
Copy link
Collaborator Author

@kategreenUSDS kategreenUSDS Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed doing this in my "make all teh fixtures use generators" PR

const benefits = useAppSelector(state => selectBenefits(state))
const incomeTotal = useAppSelector(state => selectTotalPaymentsByAllJobs(state))

function continueButtonClicked() {
router.push("/statement/sign")
}

const jobItemList = () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test broke here when i changed iterations on the job list page

@@ -140,9 +140,6 @@
"list_income_delete_button": "Delete",
"list_income_done_button": "Done",
"list_income_header": "Would you like to add another person, client, or company who paid you in the last {{day_count}} days?",
"list_income_list_header": "Your jobs and payments",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer needed

@@ -58,8 +58,16 @@ export const selectJobCount = (state: RootState) => state.jobs.allIds.length
*
* @param state
*/
export const selectTotalPaymentsByJob = (state: RootState, jobId: string) =>
selectPaymentsByJob(state, jobId).reduce((total: number, payment: PaymentItem) => total + payment.amount, 0)
export const selectTotalPaymentsByJob = (state: RootState, jobId: string) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to make this return both the data (as an object byId) and the iterator (allIds)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant