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: adding mercantile bank scraper #838

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Conversation

EzzatQ
Copy link
Contributor

@EzzatQ EzzatQ commented Mar 1, 2024

Changes:

  • Created a new scraper for bank Mercantile based on the Discount bank. The implementations are identical as both mercantile and discount use the same system. The only difference between the two is the login url (https://start.telebank.co.il/login/?bank=m). Code is based on pull request by @kfirarad.

  • I have a mercantile account and I was able to test the scrapper successfully. Manually inspecting the payload returned by scraper.scrape:

{
  "success": true,
  "accounts": [
    {
      "accountNumber": "**redacted**",
      "balance":"**redacted**,
      "txns": [
        {
          "type": "normal",
          "identifier": "**redacted**",
          "date": "2024-02-01T22:00:00.000Z",
          "processedDate": "2024-01-30T22:00:00.000Z",
          "originalAmount": -20,
          "originalCurrency": "ILS",
          "chargedAmount": -20,
          "description": ""**redacted**",
          "status": "completed"
        },
        ...
      ]
    }
  ]
}

@baruchiro
Copy link
Collaborator

Welcome @EzzatQ !!

I hope I will review it soon :-)

@EzzatQ
Copy link
Contributor Author

EzzatQ commented Mar 4, 2024

Thanks @baruchiro.

Thanks for this repo, didn't expect to find something like this but was pleasantly surprised.

I continued testing this PR and it looks like for mercantile we can only extract up to one year of transactions from current day (or actually 364 days to be exact), if requesting a longer period, mercantile's api will return an error message saying that "the requested information is not available, if you need more assistance call us on ....".

I'm wondering whether I should add some validation to the options.startDate param to avoid unexpected behavior. I can either throw an exception with a matching message, or take the maximum date between the passed options.startDate and currentDate - 364. What do you think?

@baruchiro
Copy link
Collaborator

@EzzatQ see the Discount bank as an example of limiting the startDate. It is not the only scraper that's doing the same technique:

const defaultStartMoment = moment().subtract(1, 'years').add(1, 'day');
const startDate = options.startDate || defaultStartMoment.toDate();
const startMoment = moment.max(defaultStartMoment, moment(startDate));

@baruchiro
Copy link
Collaborator

baruchiro commented Mar 5, 2024

Also, I will add you and @kfirarad to the Code Owners once we merge this, is that OK for you?
#830

@EzzatQ
Copy link
Contributor Author

EzzatQ commented Mar 5, 2024

I am aware of the defaultStartMoment solution, in fact it is implemented in the mercantile scraper because it extends the discount scraper class. What I am talking about is validation of the user input, because if I supply a date that is before 1 year prior, I get an error from mercantile api, and that error is not propagated to the user. Even if it was, the error tells me nothing about why that issue happened aka the user won't know that the scraper is failing because of an invalid start date.

I personally have no problem to be added as a Code Owner. I don't know about @kfirarad as his PR is 3 years old

@baruchiro
Copy link
Collaborator

Correct me if I'm wrong, but if you use the max date between the user startDate input and the one year ago date, the start date will never go far beyond one year, so you will never get the error from the bank, right?

@EzzatQ
Copy link
Contributor Author

EzzatQ commented Mar 6, 2024

Yes exactly

@kfirarad
Copy link

kfirarad commented Mar 7, 2024

Hi!
@EzzatQ Thanks for taking care of this one. I never had the chance to move it forward.
@baruchiro Feel free to add me as a code owner. Thank you for the suggestion.

@baruchiro
Copy link
Collaborator

Yes exactly

@EzzatQ So I don't understand when you will get the error, there will be no case for retrieving more than one year ago.

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.

Approved!

I see it is the same scraper like DiscountScraper. If it is working for you, I accept it.

@@ -1,14130 +1,8 @@
{
"name": "israeli-bank-scrapers",
"version": "1.0.4",
"lockfileVersion": 2,
"lockfileVersion": 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you didn't changed the package.json the package.lock.json should not be changed too.

I guess it is because you're using the wrong version of NodeJS, which needs to be v16.19.0.

Comment on lines +1 to +13
import DiscountScraper from './discount';

type ScraperSpecificCredentials = { id: string, password: string, num: string };
class MercantileScraper extends DiscountScraper {
getLoginOptions(credentials: ScraperSpecificCredentials) {
return {
...super.getLoginOptions(credentials),
loginUrl: 'https://start.telebank.co.il/login/?bank=m',
};
}
}

export default MercantileScraper;
Copy link
Collaborator

Choose a reason for hiding this comment

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

OMG I postponed this review for long time, just to finally review it and see it based on another scraper??

Sorry!!

@baruchiro
Copy link
Collaborator

Status: I think it is a breaking change, I waiting for @eshaham @esakal

@baruchiro baruchiro changed the title Adding mercantile bank scraper feat: adding mercantile bank scraper Mar 26, 2024
@baruchiro baruchiro merged commit 15bcd26 into eshaham:master Mar 26, 2024
5 checks passed
Copy link

🎉 This PR is included in version 4.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@EzzatQ
Copy link
Contributor Author

EzzatQ commented Mar 27, 2024

Sorry for disappearing, got very busy.
Thanks for reviewing and merging!

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.

3 participants