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.

Sign up for GitHub

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

Jump to bottom

feat: add total field to UserTopReader to get the users total amont of top reader badges #2381

Merged
merged 10 commits into from
Nov 6, 2024
Merged

feat: add total field to UserTopReader to get the users total amont of top reader badges #2381

merged 10 commits into from
Nov 6, 2024
+149 −16

Conversation

omBratteng
Copy link
Member

@pulumi Pulumi

This comment has been minimized.

Sign in to view
@pulumi Pulumi

This comment has been minimized.

Sign in to view
@omBratteng omBratteng mentioned this pull request Nov 2, 2024
Merged
2 tasks
@omBratteng omBratteng marked this pull request as ready for review November 2, 2024 00:43
@omBratteng omBratteng requested review from capJavert and a team as code owners November 2, 2024 00:43
omBratteng
omBratteng commented Nov 2, 2024
View reviewed changes
src/graphorm/index.ts Outdated
Comment on lines 207 to 223
total: {
select: (_, alias, qb) =>
qb
.select('COALESCE(COUNT(*), 0)')
.from(UserTopReader, 'utp')
.where(`utp."userId" = ${alias}."userId"`)
.groupBy('utp."userId"'),
},
Copy link
Member Author

Choose a reason for hiding this comment

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

I went with the easiest solution here, and just do count directly of the table.
Alternatively my other plan was to extend the UserStats MV to include the count of total number of badges.

Let me know if I should go the MV route

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to use MV for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored to MV

rebelchris
rebelchris approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Happy with the MV route, but for now this also works for me.

@pulumi Pulumi

This comment has been minimized.

Sign in to view
rebelchris
rebelchris approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Works for me, but wait with merging for Ante's approval please :)

capJavert
capJavert reviewed Nov 4, 2024
View reviewed changes
src/schema/users.ts Outdated Show resolved Hide resolved
capJavert
capJavert reviewed Nov 4, 2024
View reviewed changes
src/schema/users.ts Outdated Show resolved Hide resolved
capJavert
capJavert reviewed Nov 4, 2024
View reviewed changes
src/schema/users.ts Outdated Show resolved Hide resolved
@pulumi Pulumi

This comment has been minimized.

Sign in to view
sshanzel
sshanzel reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Don't have much to add. Looks great! 🚀

@pulumi Pulumi

This comment has been minimized.

Sign in to view
@pulumi Pulumi

This comment has been minimized.

Sign in to view
capJavert
capJavert approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

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

As we spoke, just watch for MV recreation, might be expensive so keep an eye out.

src/schema/users.ts Outdated Show resolved Hide resolved
@pulumi Pulumi

This comment has been minimized.

Sign in to view
@pulumi Pulumi

This comment has been minimized.

Sign in to view
@pulumi Pulumi

This comment has been minimized.

Sign in to view
@pulumi Pulumi
Copy link

pulumi bot commented Nov 5, 2024

🍹 The Update (preview) for dailydotdev/api/prod was successful.

Resource Changes

    Name                                            Type                           Operation
~   vpc-native-update-trending-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tags-str-cron                 kubernetes:batch/v1:CronJob    update
~   vpc-native-private-deployment                   kubernetes:apps/v1:Deployment  update
~   vpc-native-deployment                           kubernetes:apps/v1:Deployment  update
~   vpc-native-update-source-public-threshold-cron  kubernetes:batch/v1:CronJob    update
~   vpc-native-update-views-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-update-tag-recommendations-cron      kubernetes:batch/v1:CronJob    update
+   vpc-native-api-migration-5fc45b27               kubernetes:batch/v1:Job        create
-   vpc-native-api-migration-c22e8354               kubernetes:batch/v1:Job        delete
~   vpc-native-clean-zombie-users-cron              kubernetes:batch/v1:CronJob    update
~   vpc-native-generic-referral-reminder-cron       kubernetes:batch/v1:CronJob    update
~   vpc-native-generate-search-invites-cron         kubernetes:batch/v1:CronJob    update
~   vpc-native-hourly-notification-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-update-source-tag-view-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-sync-subscription-with-cio-cron      kubernetes:batch/v1:CronJob    update
~   vpc-native-ws-deployment                        kubernetes:apps/v1:Deployment  update
~   vpc-native-bg-deployment                        kubernetes:apps/v1:Deployment  update
~   vpc-native-check-analytics-report-cron          kubernetes:batch/v1:CronJob    update
~   vpc-native-daily-digest-cron                    kubernetes:batch/v1:CronJob    update
~   vpc-native-temporal-deployment                  kubernetes:apps/v1:Deployment  update
~   vpc-native-personalized-digest-deployment       kubernetes:apps/v1:Deployment  update
~   vpc-native-personalized-digest-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-update-highlighted-views-cron        kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-images-cron             kubernetes:batch/v1:CronJob    update
~   vpc-native-update-current-streak-cron           kubernetes:batch/v1:CronJob    update
~   vpc-native-clean-zombie-user-companies-cron     kubernetes:batch/v1:CronJob    update

@omBratteng omBratteng merged commit 04131e2 into main Nov 6, 2024
8 checks passed
@omBratteng omBratteng deleted the AS-661-profile-top-reader branch November 6, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebelchris rebelchris rebelchris approved these changes

@capJavert capJavert capJavert approved these changes

@ilasw ilasw Awaiting requested review from ilasw ilasw is a code owner automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari is a code owner automatically assigned from dailydotdev/web-team

@AmarTrebinjac AmarTrebinjac Awaiting requested review from AmarTrebinjac AmarTrebinjac is a code owner automatically assigned from dailydotdev/web-team

@sshanzel sshanzel Awaiting requested review from sshanzel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@omBratteng @rebelchris @capJavert @sshanzel