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

Claim metadata #62

Closed
wants to merge 6 commits into from
Closed

Claim metadata #62

wants to merge 6 commits into from

Conversation

marcdeb1
Copy link
Contributor

@marcdeb1 marcdeb1 commented Apr 6, 2019

@tzarebczan
Copy link
Contributor

Hey @marcdeb1, thanks for this PR...I totally missed it! We'll get a review on it shortly. There will be a few more fields we'll eventually want to add from the metadata, but they aren't available in chainquery yet.

@tzarebczan tzarebczan requested a review from tiger5226 May 28, 2019 23:58
@lbry-bot lbry-bot assigned tiger5226 and unassigned akinwale May 28, 2019
@tiger5226
Copy link
Contributor

@marcdeb1 They are actually available on dev for chainquery https://dev.chainquery.lbry.com/api/sql?query=SELECT%20*%20FROM%20chainquery.claim%20WHERE%20claim_id=%22c2bea0127e2543a5664e51cc32833851cf9dfda5%22

The commit is OdyseeTeam/chainquery@b1dab7c

you can see the sql migration to see what columns were added.

Copy link
Contributor

@tiger5226 tiger5226 left a comment

Choose a reason for hiding this comment

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

@nikooo777 Maybe we can deploy this to the dev/beta instance so we can do some live testing.

@@ -44,8 +44,9 @@ $cost = 'Free';
if (isset($claim->price) && $claim->price > 0) {
$cost = $this->Amount->formatCurrency($claim->price) . ' LBC';
} else if (isset($claim->fee) && strtolower($claim->fee_currency) === 'lbc') {
$cost = $this->Amount->formatCurrency($claim->fee) . ' LBC';
$cost = (float) ($claim->fee) . ' LBC';
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we treating currency as a float? What was wrong with formatCurrency?

}
$effective_amount = $claim->effective_amount / 100000000;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we create a const instead so the number makes sense?

src/Template/Main/claims.ctp Outdated Show resolved Hide resolved
@lbry-bot lbry-bot assigned akinwale and unassigned tiger5226 May 29, 2019
@tzarebczan
Copy link
Contributor

@marcdeb1 did you still want to finish this PR?

@tzarebczan
Copy link
Contributor

Ping @marcdeb1 :)

@marcdeb1 marcdeb1 requested a review from tiger5226 November 10, 2019 14:27
@lbry-bot lbry-bot assigned tiger5226 and unassigned akinwale Nov 10, 2019
@tiger5226 tiger5226 removed their assignment Nov 27, 2019
@tiger5226 tiger5226 removed their request for review November 27, 2019 03:40
@lbry-bot lbry-bot assigned akinwale and unassigned tiger5226 Nov 27, 2019
@tiger5226
Copy link
Contributor

I ran this locally,

  • the real time stats pages recent transactions time is still based on created_at instead of transaction time.
  • the new fields have this info type hover over that the other fields do not have.

Other than these two things, it looks good. I did not test the apis.

@kauffj
Copy link
Member

kauffj commented Dec 23, 2019

@tiger5226 ship it?

@tiger5226
Copy link
Contributor

I looked into this tonight. The environment is a bit messy. There are 3 installations. I found the one being used by caddy, but its got the blocked endpoints branch loaded there. I will deploy this after I merge the other branch. We need to fix CI too. It would be nice if it just build a docker image too and auto deployed.

@tzarebczan
Copy link
Contributor

We should try to get this in and confirm it fixes this:
image

@kauffj kauffj closed this Mar 16, 2020
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.

5 participants