mobile-wallet icon indicating copy to clipboard operation
mobile-wallet copied to clipboard

Fix #729 - Added option to filter payment history

Open dubeyaayush07 opened this issue 5 years ago • 9 comments

Fixes #729

  • [x] Apply the AndroidStyle.xml style template to your code in Android Studio.

  • [x] Run the unit tests with ./gradlew check to make sure you didn't break anything

  • [x] If you have multiple commits please combine them into one commit by squashing them.

dubeyaayush07 avatar Feb 21 '20 07:02 dubeyaayush07

Updated

Screenrecorder-2020-02-25-23-38

dubeyaayush07 avatar Feb 21 '20 08:02 dubeyaayush07

@naman14 could you review this?

dubeyaayush07 avatar Feb 21 '20 08:02 dubeyaayush07

Hey @dubeyaayush07 thanks for this PR. Seems like a good functionality to add. I had 1 comment regarding the filter logic (it should be a part of usecase so that it's not bound to the UI). Why? This is where clean architecture helps us. This allows us to plug filters dynamically later on. Maybe we start sending filterlist in api instead of filtering locally after we have already fetched all transactions. Maybe we need to save the currently selected filters in which case when users comes next time, we should only be fetching the selected filters. We also make sure that other modules are also able to use filtering if its a part of core module.

Also, maybe we can also add it at the api level. You may need to check the api docs if it supports filtering there. (Can be a separate PR) but make sure to make this a part of core usecase.

Let me know if you need more clarity.

naman14 avatar Feb 22 '20 18:02 naman14

@naman14 So instead of filtering in the presenter I should modify fetchTransactions(maybe overload the method ) to use filterList. Am I understanding it correctly?

dubeyaayush07 avatar Feb 22 '20 18:02 dubeyaayush07

Yes, pass filterList to fetchTransactions (can overload) and then pass to usecase where filtering will take place. (through request values of FetchAccountTransactions)

naman14 avatar Feb 22 '20 18:02 naman14

@naman14 I modified the code such that the filtering now takes place in the TransactionMapper. I was nervous about changing code other than that of HistoryFragment so I overloaded the fetchTransactionsHistory method. The credit, debit constants were not accessible from the the core module so for now I have hardcoded string literals.

dubeyaayush07 avatar Feb 23 '20 13:02 dubeyaayush07

@dubeyaayush07 any updates on this?

shiv07tiwari avatar Nov 10 '20 17:11 shiv07tiwari

@shiv07tiwari I am busy with my exams right now, I will try to make the necessary updates as soon as I get time

dubeyaayush07 avatar Nov 10 '20 18:11 dubeyaayush07

@shiv07tiwari @devansh-299 @naman14 In this pull request I was using a checkbox menu for the filter list but since my last commit chips were added for selecting transaction type. Should I use the menu or the chips?

dubeyaayush07 avatar Mar 03 '21 03:03 dubeyaayush07