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

Modify storage to handle expense and income #76

Conversation

yongping827
Copy link

Partially addresses #59.

JSON data files now require "transactions", "expenses" and "incomes" fields. "transactions" will be removed in a future PR. Loading and saving of current data reflects this structure.

Adding expenses and incomes does not work yet, since they are still added to transaction list, instead of expense and income lists.

@yongping827 yongping827 added priority.medium 🥈 Todo for current iteration type.enhancement 👍 New feature or request labels Oct 7, 2020
@yongping827 yongping827 added this to In progress in Fine$$e (General) via automation Oct 7, 2020
@yongping827 yongping827 added this to the v1.2 milestone Oct 7, 2020
@yongping827 yongping827 requested a review from a team October 7, 2020 16:40
@yongping827 yongping827 removed this from In progress in Fine$$e (General) Oct 7, 2020
wltan added a commit to AY2021S1-CS2103T-W16-3/reports that referenced this pull request Oct 8, 2020
Copy link
Member

@wltan wltan left a comment

Choose a reason for hiding this comment

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

Pitest report: https://ay2021s1-cs2103t-w16-3.github.io/reports/pitest/202010081253/

Note that the mutation coverage for ExpenseList and IncomeList is not very good, as the tests are based on the original AB3. It would be good if we could take a brief look to see if it can be improved, but not a hard requirement at the moment.

@yongping827 yongping827 requested a review from a team October 8, 2020 09:46
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@3e647c1). Click here to learn what that means.
The diff coverage is 88.50%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #76   +/-   ##
=========================================
  Coverage          ?   70.10%           
  Complexity        ?      450           
=========================================
  Files             ?       82           
  Lines             ?     1495           
  Branches          ?      141           
=========================================
  Hits              ?     1048           
  Misses            ?      410           
  Partials          ?       37           
Impacted Files Coverage Δ Complexity Δ
...s2103_w16_3/finesse/model/util/SampleDataUtil.java 20.00% <ø> (ø) 1.00 <0.00> (?)
...1s1_cs2103_w16_3/finesse/model/FinanceTracker.java 70.83% <55.55%> (ø) 17.00 <6.00> (?)
...3_w16_3/finesse/model/transaction/ExpenseList.java 85.71% <85.71%> (ø) 10.00 <10.00> (?)
...03_w16_3/finesse/model/transaction/IncomeList.java 85.71% <85.71%> (ø) 10.00 <10.00> (?)
...2103_w16_3/finesse/storage/JsonAdaptedExpense.java 100.00% <100.00%> (ø) 9.00 <9.00> (?)
...s2103_w16_3/finesse/storage/JsonAdaptedIncome.java 100.00% <100.00%> (ø) 9.00 <9.00> (?)
...inesse/storage/JsonSerializableFinanceTracker.java 100.00% <100.00%> (ø) 5.00 <0.00> (?)

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 3e647c1...1c9606d. Read the comment docs.

Copy link
Member

@wltan wltan left a comment

Choose a reason for hiding this comment

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

LGTM for now. As noted in the PR description more work is needed before #59 is resolved.

@yongping827 yongping827 requested a review from a team October 8, 2020 17:25
Copy link

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!
Next steps:

  1. Remodel Model and Logic such that Expense is added to ExpenseList and Income is added to IncomeList
  2. Make the correct list display in the correct tab in the UI
  3. Remove TransactionList and make Transaction an abstract class

@yongping827 yongping827 merged commit 2de446a into AY2021S1-CS2103T-W16-3:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.medium 🥈 Todo for current iteration type.enhancement 👍 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants