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

Support multiple tax years in UI, move forms to ustaxes-forms #662

Closed
wants to merge 64 commits into from

Conversation

zakpatterson
Copy link
Collaborator

@zakpatterson zakpatterson commented Oct 19, 2021

What kind of change does this PR introduce?

  • Feature

A redo of #566

Forms are now contained in ustaxes-forms. Tax years will be modeled using separate branches of that repo, and each branch will be added here as a git submodule.

Added scripts to set up submodules for 2020 and 2021.

Added the UI to enable building 2021 returns.

Data models and validations have been moved to ustaxes-core. So our structure now is:

             ustaxes-core
   /                 |       \
ustaxes-forms-2020   |     ustaxes-forms-2021
  \     (forms are single repo, multi branch)
   \                 |      / 
    \                |     / 
                ustaxes

Note: Setting up this structure did require updating NPM. This does not work on NPM 6. npm ci will also fail for users running npm6, and there is no way to warn them, since ci doesn't run any of the preinstall/prepare tasks before updating node_modules. We display an error in front of start or test.

About complexity, of course this adds a ton. If a user needs to add a new piece of UI for a new piece of user data, and update the data model, and update a tax form with that new piece of data, that will be three pull requests. On the other hand, UI PRs can't make updates to the data model or tax forms, and can't break tests in those projects. So it ...is also good? Perhaps time will tell.

@zakpatterson zakpatterson marked this pull request as draft October 19, 2021 16:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #662 (e25de85) into master (739d563) will decrease coverage by 15.31%.
The diff coverage is 60.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #662       +/-   ##
===========================================
- Coverage   71.16%   55.85%   -15.32%     
===========================================
  Files         116       66       -50     
  Lines        3815     1443     -2372     
  Branches      595      320      -275     
===========================================
- Hits         2715      806     -1909     
+ Misses       1086      624      -462     
+ Partials       14       13        -1     
Impacted Files Coverage Δ
src/components/DataPropagator.tsx 0.00% <0.00%> (ø)
src/components/Main.tsx 0.00% <0.00%> (ø)
src/components/Menu.tsx 93.33% <ø> (-0.42%) ⬇️
src/components/Patterns.ts 87.50% <0.00%> (-6.25%) ⬇️
src/components/SaveToFile.tsx 0.00% <0.00%> (ø)
src/components/Summary.tsx 45.16% <ø> (+35.48%) ⬆️
src/components/TaxPayer/PersonFields.tsx 25.00% <0.00%> (ø)
src/components/debug.tsx 0.00% <0.00%> (ø)
src/components/deductions/F1098eInfo.tsx 16.00% <ø> (ø)
src/components/income/F1099Info.tsx 56.17% <ø> (ø)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 739d563...e25de85. Read the comment docs.

@zakpatterson zakpatterson marked this pull request as ready for review October 20, 2021 09:43
@zakpatterson zakpatterson marked this pull request as draft October 20, 2021 12:42
@zakpatterson zakpatterson marked this pull request as ready for review October 24, 2021 10:39
Copy link
Collaborator

@thegrims thegrims left a comment

Choose a reason for hiding this comment

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

Has the same error as before of error messages for bad input not showing up on some pages, like the first page

@zakpatterson zakpatterson marked this pull request as draft October 25, 2021 13:05
@zakpatterson zakpatterson force-pushed the feature-years-working branch 2 times, most recently from 6627f99 to 55c75de Compare October 26, 2021 06:17
@zakpatterson
Copy link
Collaborator Author

zakpatterson commented Oct 26, 2021

    expect(received).not.toHaveLength(expected)

    Expected length: not 0
    Received array:      []

    Ignored nodes: comments, <script />, <style />

Checking out 9b62cff should show the above failed test, where the Save button is pressed and the component rerenders and resets the data without letting the errors display.

@zakpatterson zakpatterson force-pushed the feature-years-working branch 3 times, most recently from 92b0e53 to b36100e Compare October 26, 2021 13:03
@zakpatterson zakpatterson marked this pull request as ready for review October 29, 2021 02:06
@thegrims
Copy link
Collaborator

thegrims commented Oct 29, 2021

Comment has been resolved by Aidan Grimshaw

recording

Filing status should throw an error and not allow progression to the next page until it is filled out

Browser metadata
Path:      /spouseanddependent
Browser:   Chrome 95.0.4638.54 on Windows 10
Viewport:  1536 x 754 @1.25x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Unresolved

@thegrims
Copy link
Collaborator

State wages seems to sometimes throw an error when not filled out, sometimes not

recording

Browser metadata
Path:      /income/w2jobinfo
Browser:   Chrome 95.0.4638.54 on Windows 10
Viewport:  1536 x 754 @1.25x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@ustaxes ustaxes deleted a comment from netlify bot Oct 30, 2021
@netlify
Copy link

netlify bot commented Oct 30, 2021

Comment has been resolved by Aidan Grimshaw

A reviewer left a comment:

recording

I tried a ton of times and I was eventually successful in getting the form to pass without that field filled, but I think what is happening is the field was populated with "Single", which is then disallowed by the addition of a spouse. The form field removes the corresponding <option> tag so the field shows as empty, but in the form state it is still set to single.

So you should be able to see this on master now.

Browser metadata
Path:      /income/w2jobinfo
Browser:   Firefox 93.0 on Ubuntu 
Viewport:  3072 x 1080 @1.25x
Language:  en-US
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Unresolved

@zakpatterson zakpatterson marked this pull request as ready for review December 9, 2021 11:01
# Conflicts:
#	package-lock.json
#	src/components/Questions.tsx
#	src/components/RefundBankAccount.tsx
#	src/components/TaxPayer/TaxPayer.tsx
#	src/components/income/RealEstate.tsx
#	src/components/income/W2JobInfo.tsx
#	src/tests/components/Taxpayer.test.tsx
@zakpatterson zakpatterson marked this pull request as draft December 9, 2021 11:49
@zakpatterson zakpatterson marked this pull request as ready for review December 10, 2021 03:02
@zakpatterson zakpatterson marked this pull request as draft December 10, 2021 03:07
@zakpatterson zakpatterson marked this pull request as ready for review December 12, 2021 16:54
@zakpatterson
Copy link
Collaborator Author

I've finally convinced myself this strategy is a bad one. See #778

@zakpatterson
Copy link
Collaborator Author

This is replaced by #778

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.

None yet

4 participants