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 labeler info in labels and adjust color coding #61

Merged
merged 11 commits into from
Apr 17, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Mar 18, 2024

This PR fetches labeler service details and shows info about the service and the label along with any label on contents to make it easier for moderators to skim through labels.

NOTE: As a side effect of behavior based color coding, this PR removes hard coded color coding based on label categories. This is based on the fact that 3rd parties running the service won't benefit from any of the color coding and our own moderators don't seem to rely on the color coding much.

self_label

Self Label

labeler_service_details

Labeler service details

Copy link

render bot commented Mar 18, 2024

Copy link

render bot commented Mar 18, 2024

Comment on lines 504 to 511
Last{' '}
{subjectStatus.reviewState ===
ToolsOzoneModerationDefs.REVIEWNONE
? 'event'
: 'reviewed'}{' '}
at:{' '}
{dateFormatter.format(
new Date(subjectStatus.lastReviewedAt),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is lastReviewedAt set when the review state is REVIEWNONE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep.

{currentLabels.map((label) => {
const labelGroup = getLabelGroupInfo(unFlagSelfLabel(label))
{allLabels.map((label) => {
// const labelGroup = getLabelGroupInfo(unFlagSelfLabel(label))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// const labelGroup = getLabelGroupInfo(unFlagSelfLabel(label))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thought I saw this get cleaned-up too, but it's still here. Maybe some changes ended-up getting clobbered?

Comment on lines 287 to 290
<h4 className="leading-4">
<b>{label.val}</b> label does not have a custom definition. Users
will be able to configure the behavior of the label in app.
</h4>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure whether this is accurate—we might want to check in with Paul/Bryan about the right way of messaging around this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not. some labels are configurable in app though so changed the text from `will be" to "may be" which should have us covered.

Comment on lines +19 to +28
if (labelerDef?.policies.labelValueDefinitions) {
const definitionsById: Record<
string,
ComAtprotoLabelDefs.LabelValueDefinition
> = {}
labelerDef.policies.labelValueDefinitions.forEach((def) => {
definitionsById[def.identifier] = def
})
labelerDef.policies.definitionById = definitionsById
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there something in particular that we find in here that we aren't able to get from session.config.labeler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, the idea of this PR is that eventually, we could plug in other labelers into ozone. Bryan touched on this I think somewhere that other labelers may want to see labels added by bluesky.

there's a bit more work left to get this all to work and get the backend to send labels from other labelers but once that's done, this will make more sense I think.

Comment on lines +424 to +426
<ModerationLabel
recordAuthorDid={item.post.author.did}
label={label}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this needs a key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a key, the last prop.

lib/constants.ts Outdated
Comment on lines 1 to 3
export const OZONE_SERVICE_DID =
process.env.NEXT_PUBLIC_OZONE_SERVICE_DID ||
'did:plc:ar7c4by46qjdydhdevvrndac'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that we need to hardcode our labeler in Ozone. The service DID may also be set at runtime by fetching ozone-metadata.json, as is the case when running the dockerized Ozone distribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops... accidental commit, will clean up

Copy link
Collaborator

@devinivy devinivy Apr 8, 2024

Choose a reason for hiding this comment

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

@foysalit I thought I saw this get cleaned-up, but it still is kicking around here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yh idk what's up but I think I lost some changes when merging main into this.

Comment on lines +136 to 137
const builtIn = ozoneDid || OZONE_SERVICE_DID
return await getConfig(builtIn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the Ozone distribution we need to leave the possibility open that builtIn is not set, and may be determined at runtime within getConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, cleaning up the fallback of service did to our labeler should be enough for that, no?

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

The functionality here is awesome! Just a couple questions for us to answer around runtime configuration and a bit of copy in the app.

Copy link
Collaborator

@bnewbold bnewbold left a comment

Choose a reason for hiding this comment

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

this is really great! nice polish

Comment on lines 171 to 177
// Moderation events being displayed means that these events were added by the current service
// so we can assume that the src is the same as the configured ozone service DID
return (
<LabelChip key={label} style={{ color: labelGroup.color }}>
{displayLabel(label)}
</LabelChip>
<ModerationLabel
key={label}
label={{ val: label, src: OZONE_SERVICE_DID, uri: '', cts: '' }}
/>
Copy link
Collaborator

@devinivy devinivy Apr 8, 2024

Choose a reason for hiding this comment

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

I don't think we'll be able to capture the "current service" using a constant, since it may be determined at runtime. I think there's a useSession() hook kicking around that we could use to grab it, though.

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 can also just get it from client directly, right?

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Looks good!

@foysalit foysalit merged commit 4d66e7b into main Apr 17, 2024
3 checks passed
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