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

Add additional commands for Frequent Income feature #166

Conversation

siddarth2824
Copy link

@siddarth2824 siddarth2824 commented Oct 24, 2020

Resolves #158.
Resolves #159.
Resolves #160.

Changes:

  • Added EditFrequentIncomeCommand for users to edit existing frequent incomes
  • Added DeleteFrequentIncomeCommand for users to delete existing frequent incomes
  • Added ConvertFrequentIncomeCommand for users to convert the specified frequent income and add it to the finance trackers as an income
  • Added parsers for the above mentioned commands to validate user inputs
  • Added tests for:
    • EditFrequentIncomeCommand
    • DeleteFrequentIncomeCommand
    • ConvertFrequentIncomeCommand
    • EditFrequentIncomrCommandParser
    • DeletetFrequentIncomrCommandParser
    • ConvertFrequentIncomrCommandParser

Future Work:

  • Restrict users from utilising EditFrequentIncomeCommand, DeleteFrequentIncomeCommand and ConvertFrequentIncomeCommandonly under the Income tab

@siddarth2824 siddarth2824 added type.enhancement 👍 New feature or request priority.medium 🥈 Todo for current iteration labels Oct 24, 2020
@siddarth2824 siddarth2824 added this to the v1.3 milestone Oct 24, 2020
@siddarth2824 siddarth2824 requested a review from a team October 24, 2020 05:17
@codecov-io
Copy link

codecov-io commented Oct 24, 2020

Codecov Report

Merging #166 into master will increase coverage by 1.31%.
The diff coverage is 94.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #166      +/-   ##
============================================
+ Coverage     74.76%   76.08%   +1.31%     
- Complexity      701      770      +69     
============================================
  Files           124      131       +7     
  Lines          2128     2262     +134     
  Branches        226      242      +16     
============================================
+ Hits           1591     1721     +130     
+ Misses          448      447       -1     
- Partials         89       94       +5     
Impacted Files Coverage Δ Complexity Δ
...s1_cs2103_w16_3/finesse/commons/core/Messages.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...mmands/frequent/ConvertFrequentExpenseCommand.java 95.00% <ø> (ø) 8.00 <0.00> (ø)
...6_3/finesse/logic/parser/FinanceTrackerParser.java 81.03% <0.00%> (-4.43%) 31.00 <0.00> (ø)
...ava/ay2021s1_cs2103_w16_3/finesse/model/Model.java 100.00% <ø> (ø) 5.00 <0.00> (+1.00)
...equentparsers/EditFrequentIncomeCommandParser.java 91.30% <91.30%> (ø) 9.00 <9.00> (?)
...c/commands/frequent/EditFrequentIncomeCommand.java 93.33% <93.33%> (ø) 10.00 <10.00> (?)
...ommands/frequent/ConvertFrequentIncomeCommand.java 95.00% <95.00%> (ø) 8.00 <8.00> (?)
...ds/frequent/EditFrequentTransactionDescriptor.java 95.83% <95.83%> (ø) 16.00 <16.00> (?)
...commands/frequent/DeleteFrequentIncomeCommand.java 100.00% <100.00%> (ø) 7.00 <7.00> (?)
.../commands/frequent/EditFrequentExpenseCommand.java 93.33% <100.00%> (-1.12%) 10.00 <1.00> (ø)
... and 16 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 402c809...7f8fbb1. Read the comment docs.

@siddarth2824 siddarth2824 force-pushed the siddarth/frequent-income-commands branch from 82a6a8b to 76d29cd Compare October 24, 2020 05:38
wltan
wltan previously approved these changes Oct 24, 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.

I don't think it is good from a UX perspective that the user is able to convert incomes (and expenses also) from a list that is not even visible to them.

What I envision is that these commands should be restricted to their respective tabs (as is the case for edit and delete currently). Then, we can add a generic command that is simpler to use (also the case for edit and delete currently).

But that can be addressed in a future issue. The logic behind the commands LGTM!

Edit: You may want to check if #158, #159, #160 should be tagged type.story instead.

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

@wltan wltan merged commit b4f65da into AY2021S1-CS2103T-W16-3:master Oct 24, 2020
This was referenced Oct 25, 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.

Convert frequent incomes Edit frequent incomes Delete frequent incomes
4 participants