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

fixed broken leumi card adapter #219

Merged
merged 3 commits into from
Jul 15, 2019
Merged

fixed broken leumi card adapter #219

merged 3 commits into from
Jul 15, 2019

Conversation

liranay
Copy link

@liranay liranay commented Jul 12, 2019

LeumiCard changed their implementation - they now use REST calls for each search in transactions page.
The change uses puppeteer to get into leumi card website, and then use REST to get the transaction data for each month (it was scraping the UI before, which now does not exists).

Copy link
Collaborator

@esakal esakal left a comment

Choose a reason for hiding this comment

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

@liranso great job!
I left a comment regarding pending status.

description: rawTransaction.merchantName.trim(),
memo: rawTransaction.comments,
installments: getInstallmentsInfo(rawTransaction.comments),
status: TRANSACTION_STATUS.COMPLETED,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The value of status should be set to TRANSACTION_STATUS.PENDING when transactions are approved yet not charged. See below example:
processedDate is null

Consider checking processedDate and decide on status accordingly.

image

image

Copy link
Author

@liranay liranay Jul 12, 2019

Choose a reason for hiding this comment

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

@esakal
is it a proposal of a new behaviour or an old one that i did not support?
the screenshot you added shows that "paymentDate" is null, but you suggest checking "processedDate" value, please explain

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a new feature of LeumiCard,I don't think they returned pending transactions in the old API.

Now that they introduce such a use-case we must support so it is not exactly a regression nor a proposal :)

Copy link
Author

Choose a reason for hiding this comment

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

sure :)
can you explain about the the new logic you proposed?
the screenshot you added shows that "paymentDate" is null, but you suggest checking "processedDate" value

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

  const isPending = rawTransaction.paymentDate === null;
  return {
    processedDate: moment(isPending ? rawTransaction.purchaseDate :
      rawTransaction.paymentDate).toISOString(),
    status: isPending ? TRANSACTION_STATUS.PENDING : TRANSACTION_STATUS.COMPLETED,
  };

I don't think that we can leave the processedDate property value null and it make sense to use the same date of the purchase as this transaction in PENDING mode.

Copy link
Author

Choose a reason for hiding this comment

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

Done
Take a look

Copy link
Collaborator

Choose a reason for hiding this comment

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

@esakal are you sure it doesn't make sense to leave it empty? putting the purchaseDate into the processedDate will break the processedDate semantics (as the transaction wasn't actually processed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erikash, I tried to keep the same behavior as other scrapers that expose pending transactions (leumi, discount). They seems to set a date also for pending transactions. I think people expect this field to be required and their code might break at runtime once we introduce an optional as null value.

I think the term Pending is a bit misleading, as it doesn't say if its data is final and the transaction pending approval (either will remain or will be removed) or if it means that the transaction data is not final as well and might be modified once approved.

That being said, I don't might setting this value as null but in that case we should mention it in the readme.md file and consider upgrading major version to indicate breaking changes. @eshaham please let us know how you want to handle this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@esakal AFAIK pending charges amounts can be changed.. they are used to "hold" funds in cases that the final amount is not yet known (like an open tab in a hotel) or when buying gas in some gas stations... but i'm not sure about the semantics in Leumi Card :(

I liked your idea!
Maybe we can do both:

  1. Keep the contract and publish a minor
  2. In the next major change the default behavior.

@eshaham please advise...

Copy link
Owner

Choose a reason for hiding this comment

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

  1. Keep the contract and publish a minor
  2. In the next major change the default behavior.

I agree, let's not make a big change over this PR - I assume people are waiting for this scraper to be fixed ASAP, so let's get it out.

Copy link
Owner

@eshaham eshaham left a comment

Choose a reason for hiding this comment

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

@liranso @esakal I think we're missing something important here.
As I mentioned in #217 , we should make REST api calls to get the data and not navigate to the page.
So it should look more like this:

const url = getTransactionsUrl(monthMoment);
const result = await fetchGetWithinPage(page, txnsUrl);

fetchGetWithinPage is a function already included under /helpers/fetch.
This approach would make the scraper work 10 times faster, and will not require any page navigation after the login process.

@esakal
Copy link
Collaborator

esakal commented Jul 13, 2019

@eshaham indeed no need to fetch the rest content by page navigation.
@liranso can you adjust the code per @eshaham request?
Note that you should use date filter so the following methods can be unified fetchTransactionsForMonth, fetchTransactions and getCurrentTransactions

Let me know if you need my assistance.
Eran.

@liranay
Copy link
Author

liranay commented Jul 13, 2019

@esakal when i'm using "fetchGetWithinPage" i'm getting "Evaluation failed: [object Object]"
for some reason i'm not able to understand the cause of the error
i'm debugging with webstorm and defined the following node parameters - "-r babel-register"
i'm guessing it's somehow related to babel

** tester.js file in project root folder **
const {createScraper} = require('./src');

const date = new Date();
date.setMonth(date.getMonth() - 1);

(async function() {
    const scraper = createScraper({
        companyId: 'leumiCard',
        startDate: date, 
        combineInstallments: false,
        showBrowser: true,
        verbose: true,
    });
const result = await scraper.scrape({
            username: 'XYZ',
            password: 'XYZ'
        }
    )
})();

Screen Shot 2019-07-13 at 19 44 16

do you have an idea?

@esakal
Copy link
Collaborator

esakal commented Jul 14, 2019

@lielran Hi

esakal when i'm using "fetchGetWithinPage" i'm getting "Evaluation failed: [object Object]"
for some reason i'm not able to understand the cause of the error
i'm debugging with webstorm and defined the following node parameters - "-r babel-register"
i'm guessing it's somehow related to babel

I created a workable scraper. just copy&paste from this gist into your PR and see if it works for you. If it works just update your PR accordingly.

I didn't use the date filters as it seems to break the installments expected results. I created a gist with that version but ended up reverting and using the monthly scrapping concept to get the expected installments behavior.

We will then wait to @eshaham to decide about @erikash question (here) so we will be able to merge it to master soon.

Changes

[x] use fetchGetWithinPage to fetch transactions
[x] in fetchTransactions - filter months to include next month (to fetch all transactions known till today even if part of next month payment).
[x] removed unused methods

@liranay
Copy link
Author

liranay commented Jul 14, 2019

@esakal
Changed the PR
regarding this line
const allMonths = getAllMonthMoments(startMoment, true);
why do we need the next month?

@esakal
Copy link
Collaborator

esakal commented Jul 15, 2019

@esakal
Changed the PR
regarding this line
const allMonths = getAllMonthMoments(startMoment, true);
why do we need the next month?

Great, I will test it and hopefully merge it today. Did it work for you with getAllMonthMoments(startMoment, true); set to true?

To answer your question. This library scrape all transactions created till today (provided start time from user).If you set this argument to false it will not scrape next payment transactions. Let say your payment date is the 5th of each month, if you set it to false you will not get any transaction between 6/07 till today. You can easily check this out by setting this value to false if your card payment date is less then the 15th of each month (as today is 15/07). Hope this clarify the reason.

@esakal
Copy link
Collaborator

esakal commented Jul 15, 2019

@liranso it is such a small world - it was nice seeing you :)
I confirmed that it works and will merge it in the following minutes. Thanks a lot for your contribution!

@esakal esakal merged commit 08e5cad into eshaham:master Jul 15, 2019
@brafdlog
Copy link
Contributor

Thanks!

@esakal
Copy link
Collaborator

esakal commented Jul 15, 2019

@eshaham I merged the scraper into master, please deploy a new version we you can. Thanks!

@liranay
Copy link
Author

liranay commented Jul 16, 2019

@esakal :)

@brafdlog
Copy link
Contributor

@eshaham can you publish a new version to npm?

@eshaham
Copy link
Owner

eshaham commented Jul 17, 2019

@brafdlog @esakal @liranso done 👍
thanks for this great PR @liranso

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.

None yet

5 participants