-
Notifications
You must be signed in to change notification settings - Fork 554
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
✨ Optionally ack all open subjects from the author with takedown event #2793
Conversation
}, | ||
"acknowledgeAllSubjectsOfAccount": { | ||
"type": "boolean", | ||
"description": "Set to true if all subjects authored by the account should be resolved." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If true, all other reports on content authored by this account will be resolved (acknowledged)"
@@ -8,7 +8,16 @@ | |||
"parameters": { | |||
"type": "params", | |||
"properties": { | |||
"subject": { "type": "string", "format": "uri" }, | |||
"forAccount": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of feels like this should be a boolean? named allAccountSubjects
or something. you could give either a DID as subject
, or an AT-URI. either way all subjects for the account would be returned if this was true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. went with the term includeAllUserRecords
since we're using that for queryEvents.
@@ -210,6 +210,10 @@ | |||
"durationInHours": { | |||
"type": "integer", | |||
"description": "Indicates how long the takedown should be in effect before automatically expiring." | |||
}, | |||
"acknowledgeAllSubjectsOfAccount": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"acknowledgeReportedContent"? hrm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acknowledgeAccountSubjectsor
closeUnreviewedAccountSubjects`?
I don't like the verbosity either but reportedContent
doesn't feel quite right and doesn't line up with the language used in other places. `
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both of those. I'd stick with acknowledge
because that is the term we are using elsewhere and consistency is important. So acknowledgeAccountSubjects
. acknowledgeUnreviewdAccountSubjects
is longer but more specific, i'm fine with either.
statusesBefore.forEach((item) => { | ||
if (item.subject.uri === postOne.uriStr) { | ||
expect(item.reviewState).toBe(REVIEWOPEN) | ||
} else if (item.subject.uri === postTwo.uriStr) { | ||
expect(item.reviewState).toBe(REVIEWESCALATED) | ||
expect(item.appealed).toBe(true) | ||
} else if (!item.subject.uri && item.subject.did === sc.dids.bob) { | ||
expect(item.reviewState).toBe(REVIEWOPEN) | ||
} | ||
}) | ||
|
||
statusesAfter.forEach((item) => { | ||
if (item.subject.uri === postOne.uriStr) { | ||
expect(item.reviewState).toBe(REVIEWCLOSED) | ||
} else if (item.subject.uri === postTwo.uriStr) { | ||
expect(item.reviewState).toBe(REVIEWCLOSED) | ||
expect(item.appealed).toBe(false) | ||
} else if (!item.subject.uri && item.subject.did === sc.dids.bob) { | ||
expect(item.reviewState).toBe(REVIEWCLOSED) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If includeAllUserRecords
isn't working properly, these lists could be empty and cause the test to trivially pass. Potentially could select each item out of the array with something like:
const getItemBySubject = (statuses, match: { uri?: string, did?: string }) => {
return statuses.find((item) => {
if (match.did) {
return !item.subject.uri && item.subject.did === match.did
}
return item.subject.uri === match.uri
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I see it. refactored to use maps
if (isTakedownEvent && result.event.meta?.acknowledgeAccountSubjects) { | ||
await moderationTxn.resolveSubjectsForAccount(subject.did, createdBy) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest question about this work is: should the flag be tied specifically to the takedown event? It becomes a bit of a complicated idea, feels like the API reflects a fairly specific workflow, and we could probably peel these ideas apart without sacrificing much. As a bulk action, the failure modes of the acknowledgement are also pretty different from the takedown.
Since this deals with acknowledgement, I'd propose to instead add a flag to #modEventAcknowledge
, e.g. something like event.applyToAllRepoSubjects
. And then the UI would perform this bulk acknowledgement after successfully performing the takedown.
Any thoughts/feels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can totally see how adding the flag to ack event is useful and helps us decouple this from a particular event and re-use in other cases. However, I am not sure if we have such cases where we'd want to resolve all subjects without review outside of an account level takedown?
there's also this concern around surfacing these in "mod history" and while we should be able to get away with unreviewed "ack" on records if the author account is takendown but doing this in other cases might raise questions to the viewer around "why reports were acked without action on the subject".
I don't feel too strongly about either direction tbh but I like the restriction that only takedown allows us to do this seemingly aggressive ack without review.
if (!includeAllUserRecords) { | ||
builder = builder.where((qb) => | ||
subjectInfo.recordPath | ||
? qb.where('recordPath', '=', subjectInfo.recordPath) | ||
: qb.where('recordPath', '=', ''), | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When includeAllUserRecords
is true
perhaps we want qb.where('recordPath', '!=', '')
so that we only get records back. (If it's an issue it's small, wouldn't block on this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm... I do see what you mean but the name of the param doesn't really imply that ONLY records will be returned so I think I would actually expect this to include both user's account entry and record entries to be returned by this.
This PR adds a
acknowledgeAllSubjectsOfAccount
flag to takedown event which allows moderators to acknowledge all subjects in reviewOpen/reviewEscalated/Appealed state from the account being takendown.This helps clean up moderation queue of unnecessary subjects that won't require individual reviews if the author's account is already taken down permanently.
An additional but unrelated feature in this PR is a
forAccount
param intools.ozone.moderation.queryStatuses
which allows moderators to view all subject statuses for a specific account.