Get Transactions from Ledger #1578

marcogbarcellos · 2018-12-10T19:41:36Z

This PR changes the allTransactions query to support reading data either from the api OR from the ledger through either a boolean parameter on the query or an Environment variable GET_TRANSACTIONS_FROM_LEDGER .

znarf · 2018-12-11T09:36:35Z

config/development.json

+      "secret": "dvl-7749ba6ad8e62b53b18b4c30295f45d849c1",
+      "session_secret": "i&j@/V6Wx.`g>Aq?qQQF(>u)/Erm;3A="
+    }
+  },


keys added there are outdated and unnecessary.

znarf · 2018-12-11T09:36:44Z

config/development.json

+    }
+  },
+  "stripe": {
+    "platformPublishableKey": "pk_test_VgSB4VSg2wb5LdAkz7p38Gw8"


stripe added there is outdated and unnecessary.

znarf · 2018-12-11T09:39:30Z

server/graphql/v1/queries.js

        order: [['createdAt', 'DESC']],
        type: args.type,
        limit: args.limit,
        offset: args.offset,
        startDate: args.dateFrom,
        endDate: args.dateTo,
      });
+      if (!fetchDataFromLedger) return allTransactions;


Why execute and wait for allTransactions if fetchDataFromLedger is true?

I see it's used later. But why can't we fetch first from the ledger?

Humm I get your point, perhaps something fails when fetching data from the ledger and then makes it useless. you're right, I'll fix that.

znarf · 2018-12-11T09:40:36Z

server/graphql/v1/queries.js

+          }),
+        );
+      }
+      return ledgerFormattedTransactions;


Extract all this block in a separated ledger lib, which should expose a getTransactions method or similar.

znarf · 2018-12-11T09:41:40Z

server/lib/transactions.js

+  }
+  return parsedTransaction;
+}
+


That can also be added to the ledger lib.

znarf · 2018-12-11T09:43:14Z

server/graphql/v1/queries.js

    },
    async resolve(_, args) {
+      const fetchDataFromLedger = args.fetchDataFromLedger || process.env.GET_TRANSACTIONS_FROM_LEDGER || false;


If args.fetchDataFromLedger is explicitly false, it will use the value of process.env.GET_TRANSACTIONS_FROM_LEDGER. This is not what we want.

coveralls · 2018-12-11T13:50:34Z

Coverage decreased (-0.6%) to 68.474% when pulling b8f4cba on feat/ledger-get-transactions into fe6881b on master.

znarf · 2018-12-12T11:37:21Z

server/graphql/v1/queries.js

    },
    async resolve(_, args) {
+      let fetchDataFromLedger = process.env.GET_TRANSACTIONS_FROM_LEDGER || false;
+      if (args.fetchDataFromLedger) {


This won't go in the if when args.fetchDataFromLedger is false.

the default will be false which makes it ok as it'll only catch the if if the args are true, otherwise it will already be false OR defined by process.env.GET_TRANSACTIONS_FROM_LEDGER.

The way it'll be is:

the default is false

if there is process.env.GET_TRANSACTIONS_FROM_LEDGER it will go with that.

if there is args.fetchDataFromLedger it'll go with that regardless the default and process.env.GET_TRANSACTIONS_FROM_LEDGER.

No, if (args.fetchDataFromLedger) { is not checking if args.fetchDataFromLedger is undefined or not. It's checking if args.fetchDataFromLedger is truthy or falsy. If the value is false, it's not going in the condition.

If GET_TRANSACTIONS_FROM_LEDGER is true and args.fetchDataFromLedger is false, we're not having the expected behavior.

znarf · 2018-12-12T11:39:58Z

server/graphql/v1/TransactionInterface.js

        },
      },
      privateMessage: {
        type: GraphQLString,
        resolve(transaction) {
-          // TODO: Put behind a login check
-          return transaction.getOrder().then(order => order && order.privateMessage);
+          return transaction.getOrder ? transaction.getOrder().then(order => order && order.privateMessage) : null;


Why do we add all this defensive code to check if methods are available?

TransactionInterface is supposed to be expecting a Sequelize Model but when we parse the transactions coming from the ledger, those transactions are just object(with some fields like the Sequelize model) but they don't have their methods. That's why the validation is necessary.

Can you explain me why it's ok to send null there when there is no method? Aren't we expecting a value?

I've checked every return null case for those validations. I don't think we're waiting these specific values where we return null/undefined. But also in all other scenarios we have, we are using the sequelize models and the validations I added won't never be catched other than the transactionsFromLedger

… and parse them into the api format

…n case it's not a sequelize transaction object

…o instead of returning normal api transactions, return from ledger

…etchDataFromLedger' logic

…her its going to return transactions from the collectives of that host or only the transactions of the host itself

…, if so consider it and not the defaults/env vars

…e in case transaction is not a sequelize model

marcogbarcellos · 2018-12-12T15:06:13Z

@znarf after adding comments on the TransactionInterface i'll be moving forward in a couple minutes if there's nothing else! thanks for the review.

znarf reviewed Dec 11, 2018

View reviewed changes

server/lib/transactions.js Outdated

}

return parsedTransaction;

}

Copy link

Member

znarf Dec 11, 2018

Choose a reason for hiding this comment

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

That can also be added to the ledger lib.

znarf reviewed Dec 11, 2018

View reviewed changes

znarf reviewed Dec 12, 2018

View reviewed changes

marcogbarcellos mentioned this pull request Dec 12, 2018

Post transactions to ledger service(asynchronously) #1577

Merged

marcogbarcellos force-pushed the feat/ledger-get-transactions branch from 40c855c to 0dfc7d7 Compare December 12, 2018 12:26

marcogbarcellos added 10 commits December 12, 2018 09:55

feat(config): add ledger env vars and config

81fcbbe

feat(lib/transactions): add methods that gets the ledger transactions…

146d188

… and parse them into the api format

fix(graphql/TransactionInterface): validating transaction interface i…

5a33f02

…n case it's not a sequelize transaction object

feat(graphql/queries/allTransactions): add fetchDataFromLedger flag t…

a3b7344

…o instead of returning normal api transactions, return from ledger

feat(lib): create ledger lib and remove methods from transactions lib

d135669

feat(graphql/queries/allTransactions): use ledger lib and fix flag 'f…

90d3d7f

…etchDataFromLedger' logic

feat: include flag includeHostedCollectivesTransactions to check whet…

ecffeb7

…her its going to return transactions from the collectives of that host or only the transactions of the host itself

feat(lib/utils): add method to parse string or numbers to Boolean

81782e4

fix(grapqhl/queries): check whether query has arg fetchDataFromLedger…

73d1528

…, if so consider it and not the defaults/env vars

feat(grapqhl/transactionInterface): add comment to explain we validat…

b8f4cba

…e in case transaction is not a sequelize model

marcogbarcellos force-pushed the feat/ledger-get-transactions branch from 0dfc7d7 to b8f4cba Compare December 12, 2018 14:56

marcogbarcellos merged commit 8d90198 into master Dec 12, 2018

znarf deleted the feat/ledger-get-transactions branch March 21, 2019 20:19

snyk-bot mentioned this pull request Apr 27, 2021

[Snyk] Fix for 6 vulnerabilities Jankyboy/opencollective-api#38

Open

snyk-bot mentioned this pull request Aug 11, 2021

[Snyk] Upgrade redis from 3.0.2 to 3.1.2 Andrea-MariaDB-2/opencollective-api#2

Open

Bhanditz mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.3.0 to 3.2.10 Bhanditz/opencollective-api#68

Open

Bhanditz mentioned this pull request Feb 2, 2024

[Snyk] Security upgrade nodemailer from 5.1.1 to 6.9.9 Bhanditz/opencollective-api#88

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Transactions from Ledger #1578

Get Transactions from Ledger #1578

marcogbarcellos commented Dec 10, 2018

znarf Dec 11, 2018

znarf Dec 11, 2018

znarf Dec 11, 2018 •

edited

Loading

marcogbarcellos Dec 11, 2018

znarf Dec 11, 2018 •

edited

Loading

znarf Dec 11, 2018

znarf Dec 11, 2018

coveralls commented Dec 11, 2018 •

edited

Loading

znarf Dec 12, 2018

marcogbarcellos Dec 12, 2018

znarf Dec 12, 2018

znarf Dec 12, 2018

marcogbarcellos Dec 12, 2018

znarf Dec 12, 2018

marcogbarcellos Dec 12, 2018

marcogbarcellos commented Dec 12, 2018 •

edited

Loading

Get Transactions from Ledger #1578

Get Transactions from Ledger #1578

Conversation

marcogbarcellos commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znarf Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znarf Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcogbarcellos commented Dec 12, 2018 • edited Loading

znarf Dec 11, 2018 •

edited

Loading

znarf Dec 11, 2018 •

edited

Loading

coveralls commented Dec 11, 2018 •

edited

Loading

marcogbarcellos commented Dec 12, 2018 •

edited

Loading