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

nicoll/include balance transactions for reports #1761

Merged
merged 18 commits into from
Oct 23, 2024

Conversation

nicollguarnizo
Copy link
Contributor

@nicollguarnizo nicollguarnizo commented Oct 23, 2024

graph

Copy link

vercel bot commented Oct 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
badass-turbo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 0:48am
epic-react-v1 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 0:48am
epic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 0:48am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
testing-javascript ⬜️ Ignored (Inspect) Visit Preview Oct 23, 2024 0:48am
total-typescript-turbo ⬜️ Ignored (Inspect) Visit Preview Oct 23, 2024 0:48am

datalabels: {
formatter: function (value: any) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm struggling to make this loads! It seems to be a bug in quickchart, for now it displays as a number format and no the currency

https://chartjs-plugin-datalabels.netlify.app/guide/formatting.html#data-transformation

const combinedTransactions = balanceTransactionsResult.transactions.map(
(transaction): CombinedBalanceTransaction => {
if (
(transaction.type === 'charge' || transaction.type === 'payment') &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stripe added some methods of payments besides credit card, such as 'klarna' and it treats them as payment and not a charge - same with the refunds for those, so we need to add that type here.

} from 'lib/transactions'
import {prisma} from '@skillrecordings/database'
import {calculateTotals} from 'components/calculations/calculate-totals'
import {calculateSplits} from 'components/calculations/calculate-splits'

const ANNOUNCE_CHANNEL = 'C07RDAMQ7PG'
const ANNOUNCE_CHANNEL = 'C03QFFWHT7D'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will get the whole monthly report in lc-epic-web for us - it's exactly the same as our complicated spread sheet (before expenses)

const chartData = {
type: 'doughnut',
type: 'bar',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change the type of graph - a pie chart was not UX friendly when there are so many products and specially when Epic React has such an advantage over the other products, it wasn't legible. I used some min length in this type and it looks better

Copy link
Contributor

@joelhooks joelhooks left a comment

Choose a reason for hiding this comment

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

some next steps are to extract the business logic to a top level /src/reporting and think about some simple unit tests


type ProductGroup = {
export type ProductGroup = {
Copy link
Contributor

Choose a reason for hiding this comment

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

it might make sense to move these types up, maybe there's a "lib" here so we have an "sdk" for report building logic?

groups[product].fee += charge.fee
return groups
}, {} as Record<string, ProductGroup>)

// Add products that only have refunds to the productGroups
// Apply refunds to product groups
Copy link
Contributor

Choose a reason for hiding this comment

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

comment like this might suggest this could be a nice named function

@kodiakhq kodiakhq bot merged commit f0e7465 into main Oct 23, 2024
9 checks passed
@kodiakhq kodiakhq bot deleted the ng/include-balance-transactions branch October 23, 2024 16:28
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.

2 participants