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

Added credit utilization for credit cards (isracard-amex for now) #795

Closed
wants to merge 4 commits into from

Conversation

matan-elgabsi
Copy link

Not sure if this is on the roadmap, but needed to get my credit card utilization. added this quickly for isracard-amex.
Would love to hear your thoughts and correct as needed.

Thanks!

@esakal
Copy link
Collaborator

esakal commented Jul 13, 2023

@matan-elgabsi thanks for your contribution. sorry it took me time to respond.

I"m ok with the change. I have two thoughts to raise:

Thought 1

the TransactionAccount is used also for banks. maybe it would be wise to separate it like

export interface TransactionsAccount {
  accountNumber: string;
+ credit: {
  creditUtilization?: number;
  creditTotal?: number;
+  } 
  balance?: number;
  txns: Transaction[];
}

Thought 2

Do we want to always run the additional async call which if failed will fail the request and might impact response time?
If we want to make it optional we can use the option https://github.com/eshaham/israeli-bank-scrapers/blob/2f9d73638b641c2c770f104a7887f7db12800cee/src/scrapers/interface.ts#L120C3-L120C35

I'm ok with both changes, just want to hear your thoughts.
@baruchiro @erikash @matan-elgabsi wdyt?

@baruchiro
Copy link
Collaborator

Nice, why not.
Just to make sure, are you talking about מסגרת אשראי והניצול שלה?

Of course, @esakal said the right thoughts. Regarding 1: please set credit?: ... as optional to not break the code.
2 is important too.

@matan-elgabsi
Copy link
Author

Hi guys,
Thanks for the feedback!
Regarding 1:
Didn't notice that, sounds reasonable to separate them + set it as optional.
2:
I understand the problem, but not sure the flag you've mentioned is the right one, as the overhead here is just a single request.
I do think that adding some error handling and not failing the entire scraping (but instead just returning a null credit) is the right idea. What do you think?

FIY - I accidentally created the PR from a wrong account - After this discussion, I will create a new one instead.. sorry about that

@esakal
Copy link
Collaborator

esakal commented Aug 1, 2023

@matan-elgabsi sorry for the late response, my availability and access to the computer is small during Jule/Aug.

Regarding the second point, I understand it is only one call, I think the question is whether it is considered as mandatory date of that scraper or additional one. if additional then the flag fit to that purpose

@esakal
Copy link
Collaborator

esakal commented Dec 14, 2023

@matan-elgabsi hi, this is very old PR. What did you do on your end with it? is it still on your fork?
Anyhow if it is relevant we can try to finalize and add it.

@eshaham once approved we will need your help merging due to the stuck statuses tests

@baruchiro
Copy link
Collaborator

The two thoughts from the first review still need to be done to finish this PR.

@baruchiro
Copy link
Collaborator

@matan-elgabsi
Copy link
Author

Closed in favor of #857

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants