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

Adding transactions to account page #152

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Adding transactions to account page #152

merged 5 commits into from
Feb 26, 2024

Conversation

jackrua
Copy link
Contributor

@jackrua jackrua commented Feb 11, 2024

(Addressing issue #149)

Disclaimer:

You may notice more changes in files than there are actually, since I just noticed that my Editor automatically formats code so in some cases lines are just reformatted and nothing is really changed. Hence, I will try to be specific on the main changes I made to the code. However if you feel like this is an issue I can close this PR and open a new one.

Overview:

To address this issue, we need to work with both the frontend of the app (i.e. find the correct widget to represent the transactions) and its backend (i.e. retrieve from the database the transactions from a specific bank account and define the providers that the widget can watch). So, moving from bottom to top of the stack, what I did was:

  • In commit fe9eec3 I added a method to bankAccountMethods() so that I can interrogate the db to get the last n transactions from a particular account

  • In commit a3c7714 I defined the provider (selectedAccountLastTransactions) that will be exposed to the account page widget to update the transaction list.
    Note: Here I hardcoded that the widget will retrieve just the last 50 transactions (due to efficiency reasons). This is ugly, I admit, but for the time being it should be ok.

  • As a widget I chose the same widget used to represent transactions in the home page. However, I didn't want to repeat for each transaction which account belong to (since it seemed redundant to me), so I just removed in the widget the bang operator (!), hence allowing for empty names in the bank account name field (commit 78f361b)

  • Finally, I added the transaction list to the account page. Moreover, I changed the color of the back arrow, since it had the same color of the background (commit f29f92e)

@jackrua jackrua marked this pull request as draft February 11, 2024 19:38
@jackrua jackrua marked this pull request as ready for review February 12, 2024 17:40
Copy link
Collaborator

@theperu theperu left a comment

Choose a reason for hiding this comment

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

LGTM, I think we can move forward and merge this

@mikev-cw
Copy link
Collaborator

Very nice!
Just one question about this:

Note: Here I hardcoded that the widget will retrieve just the last 50 transactions (due to efficiency reasons). This is ugly, I admit, but for the time being it should be ok.

Since the graph is showing current month, it would make sense show all transactions of that month, so only those that contributes to the graph data?
I think it would be more "logical", but if you agree with this, we could handle it on another PR, and merge this meanwhile.

@mikev-cw mikev-cw linked an issue Feb 26, 2024 that may be closed by this pull request
@theperu
Copy link
Collaborator

theperu commented Feb 26, 2024

@mikev-cw I would merge this as is for now, I think it makes sense in this moment to just keep it simple and then we can think about adding filters and so on when we will be ready

@mikev-cw
Copy link
Collaborator

@theperu Ok! Let's merge therefore!

@mikev-cw mikev-cw merged commit 4a7adfc into RIP-Comm:main Feb 26, 2024
1 check passed
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.

Adding transaction list to accounts
3 participants