Skip to content

Added backend logic to allow property update#23

Open
evencewang wants to merge 6 commits into
jimmy-lan:developfrom
evencewang:feature/allow-property-update
Open

Added backend logic to allow property update#23
evencewang wants to merge 6 commits into
jimmy-lan:developfrom
evencewang:feature/allow-property-update

Conversation

@evencewang
Copy link
Copy Markdown

Added backend logic to allow property update, also updated logic for difference amount in amountChange depending on whether the old property id and the new property id are the same.
Now if a property is changed, then the amount would be deducted from the original property, and added to the new property.

@evencewang evencewang closed this Jun 14, 2022
@evencewang evencewang reopened this Jun 14, 2022
@evencewang evencewang force-pushed the feature/allow-property-update branch from 1c7f223 to a95486b Compare June 14, 2022 04:49
@jimmy-lan
Copy link
Copy Markdown
Owner

Hi Evence, thank you so much for your contributions! I have quite a few personal things that came up recently, and I'm currently resolving them, so this PR will likely be reviewed later. Thank you for your patience!

@evencewang
Copy link
Copy Markdown
Author

No worries Jimmy, wouldn't want it any other way. Make sure you settle down first!

Copy link
Copy Markdown
Owner

@jimmy-lan jimmy-lan left a comment

Choose a reason for hiding this comment

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

Hey Evence,

Thanks for the contributions.

Maybe I have a misunderstanding or am missing context, so please correct me if I'm wrong. I think to update the property associating with a transaction, we do the following:

  1. Add/Subtract the current transaction amount from the old property; and
  2. Add/Subtract a new transaction amount or the amount from the current transaction to the new property.

So I'm not sure if I understand this pull request fully, as I'm not sure if it is doing the above. Also, if you're not aware of this, there is a function in updateTransaction.ts named updatePropertyAmount, which seems to have a lot of duplicate code when it compares to the new function that you created in this pull request updateOldAndNewPropertyAmount.

@evencewang
Copy link
Copy Markdown
Author

Hi Jimmy, the difference between updatePropertyAmount and the new function is that updatePropertyAmount works for an update without a change in property. There is probably a better way to do this, but right now the old function takes in a parameter called diffAmount which is the new value minus the old value.
We don't do the same for a property change, like you said, we need to add/subtract the old amount to/from the old property, then add/subtract the new amount to/from the new property, which the newly added function does as I tested it. Would you want me to figure out a way to call the same function and pass in different things depending on whether or not there is a change to the property?

@jimmy-lan
Copy link
Copy Markdown
Owner

jimmy-lan commented Jun 30, 2022

Would you want me to figure out a way to call the same function and pass in different things depending on whether or not there is a change to the property?

No, I think the steps that I described in the previous comment might work.

  1. Add/Subtract the current transaction amount from the old property; and
  2. Add/Subtract a new transaction amount or the amount from the current transaction to the new property.

So step # 1 means to call updatePropertyAmount with the transaction amount from the old property; step # 2 means to call updatePropertyAmount with the new amount or the amount of the current transaction, if a new amount is not specified.

Not sure if this makes sense to you, but the new function updateOldAndNewPropertyAmount probably won't fit into the codebase, and it is copying plenty of things from updatePropertyAmount regardless.

@jimmy-lan jimmy-lan removed the ready label Jun 30, 2022
@evencewang
Copy link
Copy Markdown
Author

Understood, I will make the changes.

Copy link
Copy Markdown
Owner

@jimmy-lan jimmy-lan left a comment

Choose a reason for hiding this comment

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

Thanks, Evence!

I'll probably need to change a few minor places to make the code more aligned code style in this repo. I'll approve the PR for now but will merge this later.

These changes are quite minor so I don't see a value to bother you too much. I'll eventually do this myself later. I added two examples below of these things to give an idea. We can chat more offline.

@evencewang Thanks again for helping the growth of habits CLI and providing user feedback. By merging this and future pull requests, the contributor agreement that we previously agreed on will apply. If you have any questions or concerns, feel free to chat any time.

import { validateRequest } from "../../middlewares";
import { body, param } from "express-validator";
import { ResBody } from "../../types";
import { MongoDocument, ResBody } from "../../types";
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This variable seems not used

);
}
const transactionProperty = transaction.property as PropertyDocument;
let transactionProperty = transaction.property as PropertyDocument;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This change from const to let probably has no effect.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you Jimmy!

@jimmy-lan jimmy-lan added enhancement New feature or request v0.3-alpha v0.3-beta and removed v0.3-alpha labels Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request v0.3-beta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants