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

✨ Show subject stats/summary #279

Merged
merged 9 commits into from
Jan 22, 2025
Merged

✨ Show subject stats/summary #279

merged 9 commits into from
Jan 22, 2025

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Jan 16, 2025

Depends on bluesky-social/atproto#3236

This PR shows moderation stats/summary for subjects/accounts.

Screenshot 2025-01-21 at 16 18 14

Stats preview in the queue table

Screenshot 2025-01-21 at 16 18 24

Sort config for stats

Screenshot 2025-01-21 at 16 18 45

Full stats view in quick action panel

Screenshot 2025-01-21 at 16 18 52

Stats summary in quick action panel

Screenshot 2025-01-21 at 17 44 52

Stats filter view

@@ -81,6 +91,55 @@ export function SubjectTable(
)
}

const SubjectSummaryColumn = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now, we don't want to expose all stats we have. chatted with mod team and agreed that these stats may impact decisions in unwanted ways.

@arcalinea arcalinea temporarily deployed to account-summary - ozone-staging PR #279 January 21, 2025 20:59 — with Render Destroyed
Comment on lines +126 to +136
const setMinAccountSuspendCount = (minAccountSuspendCount?: number) => {
updateFilters({ minAccountSuspendCount })
}

const setMinReportedRecordsCount = (minReportedRecordsCount?: number) => {
updateFilters({ minReportedRecordsCount })
}

const setMinTakendownRecordsCount = (minTakendownRecordsCount?: number) => {
updateFilters({ minTakendownRecordsCount })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It feels like exposing both updateFilters and a helper that wraps updateFilters can be confusing when using the API. Couldn't we just require users of this hook to do:

const { updateFilters } = useQuereFilter()

updateFilters({ minTakendownRecordsCount: value })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed but we are not really using the updateFilters anywhere outside of the hook so I'll just remove it from the returned object.

Comment on lines +65 to +72
const { takedownCount, suspendCount, appealCount, reportCount } = stats
// Dont want to make the UI noisy with appeal count if we're in summary mode and already showing takedown and suspension
const shouldShowAppeal =
appealCount && (showAll || !takedownCount || !suspendCount)
// Dont want to make the UI noisy with report count if we're in summary mode and we have both takedown and suspension count
const shouldShowReport =
reportCount && (showAll || (!takedownCount && !suspendCount))
const hasStats = takedownCount || suspendCount || appealCount || reportCount
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO A zero should count as "has stats". But if that's really not what you want, this could should explicitly account for 0 vs undefined.

Suggested change
const { takedownCount, suspendCount, appealCount, reportCount } = stats
// Dont want to make the UI noisy with appeal count if we're in summary mode and already showing takedown and suspension
const shouldShowAppeal =
appealCount && (showAll || !takedownCount || !suspendCount)
// Dont want to make the UI noisy with report count if we're in summary mode and we have both takedown and suspension count
const shouldShowReport =
reportCount && (showAll || (!takedownCount && !suspendCount))
const hasStats = takedownCount || suspendCount || appealCount || reportCount
const { takedownCount, suspendCount, appealCount, reportCount } = stats
// Dont want to make the UI noisy with appeal count if we're in summary mode and already showing takedown and suspension
const shouldShowAppeal = appealCount == null
? true // No data <== I suggest we *do* show that we don't have data
: showAll ||
!takedownCount || // No data or zero
!suspendCount // No data or zero
// Dont want to make the UI noisy with report count if we're in summary mode and we have both takedown and suspension count
const shouldShowReport = reportCount == null
? true // no data <==
: showAll || (!takedownCount && !suspendCount)
const hasStats =
takedownCount != null ||
suspendCount != null ||
appealCount != null ||
reportCount != null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generally agreed but I don't think it impacts mod decisions to know that "we don't have stats" vs. the stats is 0 and I'm trying to be cautious about making the UI too noisy with stats.
at least to start with, I see these as additional metadata to help enforce decisions made from other factors already rather than a primary factor for making decision.

)
}

export const RecordsStats = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I suggest we

  1. make a clear distinction between 0 and undefined, to make the business logic clearer in the code
  2. that we do not use undefined as a reason not to show something (using a question mark in that case)

@foysalit foysalit merged commit 475c934 into main Jan 22, 2025
3 checks passed
@matthieusieben matthieusieben deleted the account-summary branch January 23, 2025 10:00
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.

3 participants