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

feat: fetch credit card line of credit utilization #857

Conversation

matanelgabsi
Copy link
Contributor

Optional feature to fetch credit card line of credit utilization and limit.
Initial support for Isracard-amex, Cal, Max.
This is a replacement PR of #795.
The main changes from #795:

  1. Added Max & Cal
  2. Fixed comment regarding returned data type (Thanks @esakal)
  3. Added a parameter to control whether credit utilization is collected (Thanks again @esakal)

Sorry for the long delay doing this, hope this answers your concerns and we can merge :)

Optional feature to fetch credit card line of credit utilization and limit.
Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

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

Please see my comments.

I don't know why did you open a new Pull Request instead of continuing #795, but please don't do it again. If you need help with working with Pull Requests, please ask me.

src/scrapers/base-isracard-amex.ts Show resolved Hide resolved
Comment on lines +64 to +67
interface AccountCredit {
creditUtilization: number;
creditLimit: number;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can declare this interface once in transactions.ts and import it everywhere.

Choose a reason for hiding this comment

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

I though about that too, but as this is an internal type (it's used locally, not "passed" outside the file) I opted into this.
If you think otherwise - let me know.

Comment on lines +370 to +375
if (creditUtilization && creditUtilization[accountNumber]) {
account.credit = {
creditUtilization: creditUtilization[accountNumber].creditUtilization,
creditLimit: creditUtilization[accountNumber].creditLimit,
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about this?

Suggested change
if (creditUtilization && creditUtilization[accountNumber]) {
account.credit = {
creditUtilization: creditUtilization[accountNumber].creditUtilization,
creditLimit: creditUtilization[accountNumber].creditLimit,
};
}
account.credit = creditUtilization?.[accountNumber];
  1. The creditUtilization[accountNumber] structure is the same as the account.credit. Even if it is null or undefined it is OK because we accept no response for some accounts.

Choose a reason for hiding this comment

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

Looks like a better idea, Will update.

async function fetchCreditUtilization(
page: Page,
servicesUrl: string,
): Promise<_.Dictionary<AccountCredit> | null> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use standard types, I'm not familiar with lodash types, I can't understand them from the tooltip and I don't expect other contributors to understand it.

Choose a reason for hiding this comment

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

As lodash is already installed on the project I believed it's OK.
Will change

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, although I'm thinking a lot before I add lodash to a project, if it is already there I accept using it.

But still, for the function signature, which is a kind of contract/interface, I prefer to use standard types.

@baruchiro
Copy link
Collaborator

Please pull the latest changes from master (@eshaham can you enable the button for "Update Branch"?)

@eshaham
Copy link
Owner

eshaham commented Jun 11, 2024

Please pull the latest changes from master (@eshaham can you enable the button for "Update Branch"?)

@baruchiro good idea, done 👍

@matan-elgabsi
Copy link

@baruchiro I commented on the previous PR saying I will replace it right after I've opened it, the reason is I accidentally committed from my company's account, this is why I needed a new PR from my personal account.

Copy link

Pull request has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Nov 20, 2024
@github-actions github-actions bot closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants