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

use DB connector instead of web API #158

Merged
merged 11 commits into from Oct 31, 2019

Conversation

@nikooo777
Copy link
Member

nikooo777 commented Jul 8, 2019

using the web API is not smart. this PR fixes that issue

@kauffj kauffj self-requested a review Jul 9, 2019
@lbry-bot lbry-bot assigned kauffj and unassigned nikooo777 Jul 9, 2019
Copy link
Member

tiger5226 left a comment

We cannot remove that logging. It's important that we see that. Otherwise, I did not test this out with the app. I know you said it is pretty solid. Maybe @tzarebczan can take a look too in dev to make sure its good to go.

status.info = 'addingClaimsToElastic';
for (let claim of claims) {
if (claim.value === null) {
console.log(claim);
await logErrorToSlack('Failed to process claim ' + claim.claimId + ' due to missing value');
// await logErrorToSlack('Failed to process claim ' + claim.claimId + ' due to missing value');

This comment has been minimized.

Copy link
@tiger5226

tiger5226 Jul 10, 2019

Member

I put this here intentionally. I need to know when they fail so I can make sure I fix any data in chainquery. We should however, make sure its not hardcoded and only production reports.

effective_amount : r.effective_amount,
certificate_amount: r.certificate_amount,
claimId : r.claimId,
value : r.value,

This comment has been minimized.

Copy link
@tiger5226

tiger5226 Jul 10, 2019

Member

I don't know if this is right brother. I needs to be pushed as a json string for it to work properly. I would need to test this. However, I think you think this is pretty solid.

Copy link
Member

kauffj left a comment

Just some best practice suggestions in a file that doesn't look like it's adhering too closely to them in the first place 😛

var slackAPIKey = process.env.SLACK_HOOK_URL;
var mySlack = new slack(slackAPIKey, {});
let slackAPIKey = process.env.SLACK_HOOK_URL;
let mySlack = new slack(slackAPIKey, {});

This comment has been minimized.

Copy link
@kauffj

kauffj Jul 10, 2019

Member

const is technically more appropriate than let here but it doesn't really matter

@@ -168,33 +172,45 @@ function getBlockedOutpoints () {
});
}

let connection = null;

This comment has been minimized.

Copy link
@kauffj

kauffj Jul 10, 2019

Member

these lines should probably go at top of file

function getClaimsSince (time, lastID, MaxClaimsInCall) {
if (connection === null) {

This comment has been minimized.

Copy link
@kauffj

kauffj Jul 10, 2019

Member

A slightly better pattern would be:

connection = getConnection()

function getConnection() {
  //instantiate singleton here
}
return reject(err);
}
let claims = [];
for (let i = 0; i < results.length; i++) {

This comment has been minimized.

Copy link
@kauffj

kauffj Jul 10, 2019

Member

Not strictly necessary but you can use Array.map for this:

const claims = results.map(r => {
          id                : r.id,
          name              : r.name,
          channel           : r.channel,
          bid_state         : r.bid_state,
          effective_amount  : r.effective_amount,
          certificate_amount: r.certificate_amount,
          claimId           : r.claimId,
          value             : r.value
})
@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Sep 18, 2019

@nikooo777 what's the status of this PR? Sitting out here a while.

@nikooo777

This comment has been minimized.

Copy link
Member Author

nikooo777 commented Sep 19, 2019

I have to find time to clean it up... I'll try to get that done asap.

@nikooo777 nikooo777 force-pushed the chainquery-db-connection branch from 75e56d3 to 0810838 Sep 20, 2019
@nikooo777 nikooo777 force-pushed the chainquery-db-connection branch from 0810838 to 70afbee Oct 2, 2019
@nikooo777 nikooo777 force-pushed the chainquery-db-connection branch from 88335c2 to 9934258 Oct 14, 2019
nikooo777 added 3 commits Oct 14, 2019
index channel claim id for claims
@nikooo777 nikooo777 merged commit 34df58e into master Oct 31, 2019
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.