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
Add reporting feature #5155
Add reporting feature #5155
Conversation
@@ -33,6 +33,8 @@ | |||
<PackageReference Include="BTCPayServer.Lightning.Common" Version="1.3.21" /> | |||
<PackageReference Include="NBitcoin" Version="7.0.24" /> | |||
<PackageReference Include="Newtonsoft.Json" Version="13.0.1" /> | |||
<PackageReference Include="NodaTime" Version="3.1.9" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a whole new library on the client library level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to remove it, we dont
} | ||
}; | ||
|
||
$(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce jquery usage:
$(function () { | |
document.addEventListener("DOMContentLoaded", () => { |
}; | ||
|
||
$(function () { | ||
$(".flatdtpicker").on("input", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(".flatdtpicker").on("input", function () { | |
document.querySelectorAll('.flatdtpicker').forEach(el => el.addEventListener("input", ()=>{ |
$(function () { | ||
$(".flatdtpicker").on("input", function () { | ||
// We don't use vue to bind dates, because VueJS break the flatpickr as soon as binding occurs. | ||
var to = $("#toDate").val(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var to = $("#toDate").val(); | |
const to = document.querySelector("#toDate").value |
What is the problem with jquery? |
@dstrukt @dennisreimann I just added a commit to generate fake data in the reporting.
|
I think we could either put it up between Dashboard and Settings or at the end of the Payments section. Regardless of where we position it, we need a new icon for this feature :) @dstrukt |
The make me more happy |
// filter(){ return filter1 && filter2 && filter3; } | ||
var newData = []; | ||
var o = {}; | ||
eval('function filter() {return ' + filterStrings.join(' && ') + ';}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The basic idea is that the report provider can decide to filter rows before building the table.
I use it for the summary of lightning addresses payments. Without this, the datatable would show a row with empty ligthning address if none are present. Instead, I want this line to be gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the conversion from tabular data to a representation bindable to VueJS for summary tables is done JS side.
I am unsure this is good idea, I believe we could just do that C# side.
It would also make it easier to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few initial thoughts after reviewing, some might be out of scope, others are minor nits and suggestions:
Happy to make some of these changes post-merge as well, if it makes more sense.
Search
- Do we want the ability to search? e.g. something like this:
- Calendar icons to the left of the inputs for the date selection could be nice too
Balance Tables
- Any reason we don't want to conserve a bit of space by doing two columns like so. Am I missing something? Might be nice to consolidate in the future, but for the time being.
Payments
- Could be improved with updated labels:
- Lightning Address label updates
- Makes me think some of the other Amount labels could be Balance instead...hmm
On-Chain
- Could be improved with updating the label and pushing the header into the table key value for the on-chain view:
- Context: I found the Balance Change aspect of the On-Chain table a little confusing... as the test data amounts were mostly negative values?
- Still exploring ways of consolidating the tables for more space and ease of parsing the data, just sharing, might be a later follow up if any of the explorations make sense:
e.g: On-Chain/Lightning, doesn't hold up as well, but interesting nonetheless:
works in a simple context as well:
Products
-
Summary by items
->Summary
Could this label just be shortened, and still make sense? - Should it be App ID or the App/Plugin Name? Or perhaps both?
Raw Data
Payments
- Is the date formatting messed up?
2023-07-05T01:42:41-06:00
On-Chain
- Suggestions for improvements
- Unconfirmed would be a red
X
icon instead of the green checkmark - Balance Change decrease would be
-1.06150021
and be colored red
Filter Dropdown
- Copy suggestion:
Items sold
->Products
orProducts Sold
? To keep it consistent with the POS label - Copy suggestion:
On-Chain Wallets
->On-Chain
orWallets
? On second thought, might be fine with the original..
General
- I noticed there's an additional scrollbar at the bottom of the page, can we remove this? I think the table specific scrollbar makes sense, given the width of it's contents.
- The main CTA could arguably just be
Export
cleaner, as the CSV bit isn't necessary imo
Really great work!
About wallet balance confirmed/unconfirmed: I don't believe a bar make sense. In most case, unconfirmed will be ridiculously low compared to confirmed, so this doesn't add lot's of information. For on-chain versus lightning, it might make more sense. Also, I was thinking: Some tables are grouping on Invoice currency. I believe this is overkill, as most of the time, the invoice currency is the same. Rather than grouping per currency, I will just make additional chart per currency? |
@dstrukt I want to add, the aggregated tables I am showing could potentially be replaced by any chart that make sense. |
values.Add((string)r.order_id); | ||
if (PaymentMethodId.TryParse((string)r.payment_type, out var paymentType)) | ||
{ | ||
if (paymentType.PaymentType == PaymentTypes.LightningLike || paymentType.PaymentType == PaymentTypes.LNURLPay) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the payment type tostring instead of hardcoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because because the difference between lightning versus LNURL doesn't matter at all for merchants.
They want the consolidated view of how much they received on lightning, they don't care whether it come from LNURL or not, so we need to map them to the same value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing api docs and tests! 😂
Missing tests, will do. For the API, I will remove it from this PR, exposing it as a |
Tend to agree, it's a bit much for what would only be a pending state at best, and rarely visible for instances with low volume.
Also agree, but wasn't intended to be included in this PR, just a quick visual exploration to see if it made sense / worth the time to implement!
That could work, I do appreciate the flexibility/clarity of the current implementation, but I think I'd have to see it. |
@NicolasDorier Any chance fake data can generate items for POS? I think this is one of the highest-requested reports, so would be good to test it out. |
ACK to fake data, if possible! Was my biggest challenge in providing more feedback as well. |
@NicolasDorier I've tested draft a bit, so far so good. We're thinking of RC for Monday, but unsure how much you have to do on this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks really solid, there's some more finessing of labels and UI I'd like to do, but I don't want to hold up the release to make these minor tweaks.
Especially if our RC deadline is monday, this is something @dennisreimann and I could easily do a follow up PR to address.
it's bit tight, so let's merge this one and incrementally improve on dstrukt feedback |
@pavlenex I don't know what you mean, but the CSV export is exporting the raw data, because all other structures can be trivially created again through Excel. The idea is to provide just enough feature to be useful for simple cases, delegating complex cases to Excel. Filtering for now is only on date, I believe allowing some simple client side filters on the raw data. About Grouping: The summary tables that you see is grouping. If what you want is letting the user drag and drop components to make their own group, I wished to do that but the existing open source components for pivot table just sucks and aren't maintained anymore. |
Sounds good - will make an issue with some of the remaining items post-release. Great work! |
Good |
Features
Out of scope
While the Greenfield route has been done, I think we should disable it for now, as we are still unsure of proper API design.
Additional reports will be in different PRs after we could validate the design of this PR.
To do later
UX
This add a new page at store level: Reporting:
When clicking on it, a new page open
Note on the UX: The hard part is that the designer is unable to predict how many columns will be there. That said, cell content can be customized depending on the type of the column.
Implementation details
Backend
A new class
ReportService
is exposing report related methods.A base class
ReportProvider
can be subclassed to implement new types of reports.At the moment, only two implementations exists
PaymentsReportProvider
andOnChainWalletsReportProvider
, but many more will come for next major release.Front end
I am using VueJs, the whole page is coded as a single page application.
Linked discussions
#5095
#937
#2561
#2601
#4598