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

Made evil comments private and their donations marked for moderation #490

Merged
merged 2 commits into from Jun 13, 2016

Conversation

3 participants
@JeroenDeDauw
Member

JeroenDeDauw commented Jun 10, 2016

@gbirke gbirke merged commit 18f807c into master Jun 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gbirke gbirke deleted the commentmoderation branch Jun 13, 2016

@@ -46,6 +51,10 @@ public function addComment( AddCommentRequest $addCommentRequest ): AddCommentRe
$donation->addComment( $this->newCommentFromRequest( $addCommentRequest ) );
if ( !$this->commentTextPassesValidation( $addCommentRequest->getCommentText() ) ) {

This comment has been minimized.

@KaiNissen

KaiNissen Jun 13, 2016

Contributor

In cases where users want to publish their comment the validation is being carried out twice.

This comment has been minimized.

@JeroenDeDauw

JeroenDeDauw Jun 13, 2016

Member

I know. I did not see a way to do it only once and still have both checks happen, without adding mutable state. My thinking here is that the code is slightly simpler without the state, and that the performance difference is not relevant (no expensive resources are used). If you disagree with the later, or see a way to do it once without making the code more complicated, I'd love to hear it.

This comment has been minimized.

@KaiNissen

KaiNissen Jun 13, 2016

Contributor

We could store the validation result in a private field and that if the validation was already done. True, the use case object wouldn't be immutable anymore (at least not internally).

I'm just saying, because the validator also checks DNS entries for URLs included in a comment, which would increase the server response time if executed twice. Not really opposing this PR as is, though.

This comment has been minimized.

@JeroenDeDauw

JeroenDeDauw Jun 13, 2016

Member

As far as I understood the code, the DNS entries are not checked. It's checking URLs, but not doing the DNS check.

This comment has been minimized.

@KaiNissen

KaiNissen Jun 13, 2016

Contributor

Right. The old application did, though. As far as I remember, we introduced that a while back, because there were false positives produced by punctuation mistakes. What looks like a URL doesn't necessarily have to be one.is what I mean.

$request->getAuthorDisplayName()
);
}
private function commentCanBePublic( AddCommentRequest $request ): bool {
return $request->isPublic()
&& $this->commentTextPassesValidation( $request->getCommentText() );

This comment has been minimized.

@KaiNissen

KaiNissen Jun 13, 2016

Contributor

Again, this removes the original intent of users whether or the comment should be public. What about we add another filter for moderated (and deleted/cancelled) donations in the CommentRepository to prevent that stuff from ending up in the public list of comments?

This comment has been minimized.

@JeroenDeDauw

JeroenDeDauw Jun 13, 2016

Member

Works for me. I suggest to create a ticket for that under New Stuff. One question I have for it is: does the backend need to be modified in any way to match such a change?

@gbirke

This comment has been minimized.

Member

gbirke commented Jun 13, 2016

Described how this can be solved in https://phabricator.wikimedia.org/T137704
@KaiNissen please check if the task description is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment