Skip to content

feat(site): display user status history as an indication of license usage #16020

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
Closed
wants to merge 23 commits into from
Closed

feat(site): display user status history as an indication of license usage #16020

wants to merge 23 commits into from

Conversation

SasSwart
Copy link
Contributor

@SasSwart SasSwart commented Jan 3, 2025 •
edited
Loading

RE: #15740, #15297

Based on #16019, this PR adds the frontend changes required to display a graph of user status over time in the general settings page.

SasSwart added 20 commits January 3, 2025 08:35
@SasSwart
add user_status_changes table
4cd66af
@SasSwart
add GetUserStatusCountsByDay
61b27bf
@SasSwart
rename unused variable
7cba28c
@SasSwart
Test GetUserStatusCountsByDay
2d748c9
@SasSwart
make gen
52620ce
@SasSwart
fix dbauthz tests
6d15a32
@SasSwart
do the plumbing to get sql, api and frontend talking to one another
b99468f
@SasSwart
rename migration
b204783
@SasSwart
move aggregation logic for GetUserStatusChanges into the SQL
ce1bda5
@SasSwart
use window functions for efficiency
734ff2b
@SasSwart
ensure we use the same time zone as the start_time param
737e63e
@SasSwart
ensure we use the same time zone as the start_time param
045e438
@SasSwart
make gen
32aeb4d
@SasSwart
update field names and fix tests
1a2fde8
@SasSwart
exclude deleted users from the user status graph
5b8fcac
@SasSwart
GetUserStatusChanges now passes all querier tests
4ac83aa
@SasSwart
renumber migrations
b23018a
@SasSwart
add partial fixture for CI
adbc8aa
@SasSwart
fix migration numbers
8bff303
@SasSwart
rename and document sql function
1ceff06
@SasSwart SasSwart changed the base branch from main to jjs/dau-history-backend January 3, 2025 08:39
@SasSwart SasSwart changed the base branch from jjs/dau-history-backend to main January 3, 2025 08:39
@SasSwart SasSwart changed the title Jjs/dau history frontend feat(site): Display user status history in the general settings page Jan 3, 2025
@SasSwart SasSwart changed the title feat(site): Display user status history in the general settings page feat(site): Display user status history in the general settings page as an indication of license usage Jan 3, 2025
@SasSwart SasSwart changed the title feat(site): Display user status history in the general settings page as an indication of license usage feat(site): display user status history as an indication of license usage Jan 3, 2025
@SasSwart SasSwart requested a review from BrunoQuaresma January 3, 2025 09:35
@SasSwart SasSwart marked this pull request as ready for review January 3, 2025 10:02
@BrunoQuaresma
Copy link
Collaborator

If I want to test this locally, what’s the best way to generate the data? I’d really appreciate it if you could share a seed script so that I—and possibly a PM—can fully test it before it’s merged.

@BrunoQuaresma
Copy link
Collaborator

@SasSwart, I’d also suggest requesting a review from someone with BE expertise, as there are quite a few changes in this PR.

BrunoQuaresma
BrunoQuaresma reviewed Jan 3, 2025
View reviewed changes
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

FE code looks solid—great work! I'll hold off on approving until I test the changes locally. Sounds good?

site/src/components/ActiveUserChart/ActiveUserChart.tsx
<HelpTooltipTitle>How do we calculate active users?</HelpTooltipTitle>
<HelpTooltipTitle>
How do we calculate user activity?
</HelpTooltipTitle>
<HelpTooltipText>
When a connection is initiated to a user&apos;s workspace they are
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we’re updating the data on this chart, should we also update the content of this tooltip?

@SasSwart
Copy link
Contributor Author

SasSwart commented Jan 3, 2025 •
edited
Loading

Yeah. That's perfect. The backend changes are being reviewed in a separate PR. Once that merges this will be FE only. No rush. Thanks!

@BrunoQuaresma
Copy link
Collaborator

BrunoQuaresma commented Jan 3, 2025 •
edited
Loading

Considerations for Chart Design (Based on Storybook Stories)

GeneralSettingsPageView: Many Users

Screenshot 2025-01-03 at 08 52 09
  1. Color Representation:
    It’s unclear what each color represents in the chart. Consider adding a legend or including the color associations in the tooltip next to the corresponding labels.

ActiveUserChart: Multiple Series

Screenshot 2025-01-03 at 08 53 34

  1. Red Line Meaning:
    What does the red line represent in this chart? If it’s significant, this should be clarified visually or through the tooltip.

  2. "Active User Limit" Line:
    The purpose of this line is unclear. Should users be concerned about this limit? If yes, how should they interpret or act on it?
    cc: @matifali @bpmct

GeneralSettingsPageView: Total Users Exceeds License But Not Active Users

Screenshot 2025-01-03 at 08 52 32
  1. Contrast Issue:
    The "Active User Limit" line lacks sufficient contrast and becomes hard to distinguish against the other lines, particularly for users with visual impairments.

In #15297, we discussed how this chart should ideally look. Over time and through various iterations, the original vision may have been lost. I still believe that aligning the design closer to the initial proposal would enhance its clarity and professionalism.

Achieving this may require installing the shadcn Chart component manually and addressing potential integration issues.

@SasSwart, I understand this might extend beyond your typical scope, so I’m happy to take care of it for you if you’d prefer. However, if you’d like to explore this yourself, I’d be more than happy to guide you through the process. Additionally, we can create a separate issue to address these enhancements once this PR is merged. Let me know how you’d like to proceed! 😊

@github-actions github-actions bot added the stale This issue is like stale bread. label Jan 11, 2025
BrunoQuaresma
BrunoQuaresma approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving this so we can merge it into main ASAP. I’ll refine the design in a separate PR.

@SasSwart SasSwart closed this Jan 13, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrunoQuaresma BrunoQuaresma BrunoQuaresma approved these changes

Assignees

@SasSwart SasSwart

Labels
stale This issue is like stale bread.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SasSwart @BrunoQuaresma