harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Allow moderation of public article comments #354

Merged
matthijskooijman merged 7 commits from article-moderation into master 2020-11-17 14:25:48 +00:00
Conversation 11 Commits 7 Files changed 6 +409 -14
matthijskooijman commented 2020-10-26 18:37:10 +00:00 (Migrated from github.com)
Copy link
No description provided.
matthijskooijman commented 2020-10-29 14:38:36 +00:00 (Migrated from github.com)
Copy link

I further improved this PR, replacing the predefined reasons with a free-form input for reasons (which does remove the translations), allowing comments to be deleted (and undeleted) as well, and made the editing interface a bit more adaptive (i.e. change wordings based on whether a comment is already moderated or not).

There are a few commits that modify HTMLForm, adding methods related to select dropdowns, which are now no longer used. But since they're likely to be useful in the future, I've left these commits in the PR.

I did a force push for easier review of the complete changes.

I further improved this PR, replacing the predefined reasons with a free-form input for reasons (which does remove the translations), allowing comments to be deleted (and undeleted) as well, and made the editing interface a bit more adaptive (i.e. change wordings based on whether a comment is already moderated or not). There are a few commits that modify HTMLForm, adding methods related to select dropdowns, which are now no longer used. But since they're likely to be useful in the future, I've left these commits in the PR. I did a force push for easier review of the complete changes.
laurensmartina (Migrated from github.com) requested changes 2020-10-31 08:32:53 +00:00
system/core/document.php
@ -499,0 +510,4 @@
throw new LogicException("Form field \"$name\" not found");
else if ($found->count() > 1)
throw new LogicException("Form field \"$name\" found multiple times");
laurensmartina (Migrated from github.com) commented 2020-10-31 08:14:15 +00:00
Copy link

use type hinting at $field.

use type hinting at `$field`.
laurensmartina (Migrated from github.com) commented 2020-10-31 08:15:15 +00:00
Copy link

and also at $attributes

and also at `$attributes`
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-10-31 08:21:45 +00:00
Copy link

use type hinting

use type hinting
laurensmartina (Migrated from github.com) commented 2020-10-31 08:23:36 +00:00
Copy link

use strikt comparison !==

use strikt comparison `!==`
laurensmartina (Migrated from github.com) commented 2020-10-31 08:28:49 +00:00
Copy link

the name of this variable is a bit confusion, it's not always hidden.

the name of this variable is a bit confusion, it's not always hidden.
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-10-31 08:30:03 +00:00
Copy link

remove the second blank line

remove the second blank line
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-10-31 08:16:29 +00:00
Copy link

use typehinting

use typehinting
system/datatypes/peer_reviewed_article.php
@ -1109,0 +1324,4 @@
$li->addClass('hidden');
$details = $this->html->createElement('details')->appendTo($li);
$placeholder = $this->html->createElement('summary')->appendTo($details);
$commentHtml->appendTo($details);
laurensmartina (Migrated from github.com) commented 2020-10-31 08:20:41 +00:00
Copy link

$resolved is never passed

`$resolved` is never passed
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-10-31 08:27:21 +00:00
Copy link

It would be more diverse if the class would be separated so that all comments get the "comment" class and the first comment would get "first" as well.

It would be more diverse if the class would be separated so that all comments get the "`comment`" class and the first comment would get "`first`" as well.
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 14:47:11 +00:00
system/core/document.php
@ -499,0 +510,4 @@
throw new LogicException("Form field \"$name\" not found");
else if ($found->count() > 1)
throw new LogicException("Form field \"$name\" found multiple times");
matthijskooijman (Migrated from github.com) commented 2020-10-31 14:47:10 +00:00
Copy link

Right, I did the comments, but then it is still useful to use PHP-readable hints as well, of course.

Fixed.

Right, I did the comments, but then it is still useful to use PHP-readable hints as well, of course. Fixed.
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 14:51:31 +00:00
system/datatypes/peer_reviewed_article.php
@ -1109,0 +1324,4 @@
$li->addClass('hidden');
$details = $this->html->createElement('details')->appendTo($li);
$placeholder = $this->html->createElement('summary')->appendTo($details);
$commentHtml->appendTo($details);
matthijskooijman (Migrated from github.com) commented 2020-10-31 14:51:31 +00:00
Copy link

w00ps, fixed :-)

w00ps, fixed :-)
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 14:51:36 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-10-31 14:51:35 +00:00
Copy link

Fixed.

Fixed.
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 14:54:22 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-10-31 14:54:22 +00:00
Copy link

Maybe, or maybe using :first-child could remove the need for this class entirely. However, this just moves the existing class, so I'm going to consider this out of scope for the PR for now.

Maybe, or maybe using `:first-child` could remove the need for this class entirely. However, this just moves the existing class, so I'm going to consider this out of scope for the PR for now.
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 14:58:49 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-10-31 14:58:49 +00:00
Copy link

Do you mean that "hidden" is confusing in general, because you can still show the comment by expanding it? This is true, but I couldn't think of any better name ("hidden-by-default" is too complicated, I think). And it is hidden, it's just also
possible to reveal what's hidden :-)

Or do you mean that this particular button does not always hide the comment, it sometimes only changes the reason? That's not really what happens in practice, though, since this button always uses the HIDE command, so it always sets the moderate-status to hidden and sets the moderate-reason. So if the status is already hidden, this (in the view of the user) only changes the reason, but underwater it still does both.

Maybe this clarifies things? If not, let me know what you mean exactly, and maybe you also have a suggestion to improve?

Do you mean that "hidden" is confusing in general, because you can still show the comment by expanding it? This is true, but I couldn't think of any better name ("hidden-by-default" is too complicated, I think). And it *is* hidden, it's just also possible to reveal what's hidden :-) Or do you mean that this particular button does not always hide the comment, it sometimes only changes the reason? That's not really what happens in practice, though, since this button always uses the HIDE command, so it always sets the moderate-status to hidden and sets the moderate-reason. So if the status is already hidden, this (in the view of the user) only changes the reason, but underwater it still does both. Maybe this clarifies things? If not, let me know what you mean exactly, and maybe you also have a suggestion to improve?
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 15:00:32 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-10-31 15:00:32 +00:00
Copy link

Fixed.

Fixed.
matthijskooijman (Migrated from github.com) reviewed 2020-10-31 15:00:49 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2020-10-31 15:00:48 +00:00
Copy link

Fixed.

Fixed.
laurensmartina (Migrated from github.com) approved these changes 2020-11-17 12:19:00 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-11-17 12:16:46 +00:00
Copy link

I think I misread the $hidden = $comment->getAttribute(self::FIELD_NAME_DISCUSSION_MODERATION_STATUS) === self::MODERATION_STATUS_HIDDEN; for $hidden = $comment->getAttribute(self::FIELD_NAME_DISCUSSION_MODERATION_STATUS);.
My bad.

I think I misread the `$hidden = $comment->getAttribute(self::FIELD_NAME_DISCUSSION_MODERATION_STATUS) === self::MODERATION_STATUS_HIDDEN;` for `$hidden = $comment->getAttribute(self::FIELD_NAME_DISCUSSION_MODERATION_STATUS);`. My bad.
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2020-11-17 12:13:31 +00:00
Copy link

agreed

agreed
matthijskooijman commented 2020-11-17 14:25:43 +00:00 (Migrated from github.com)
Copy link

Thanks! I squashed the fixups and will merge now.

Thanks! I squashed the fixups and will merge now.
Sign in to join this conversation.
Reviewers
No reviewers
laurensmartina
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!354
No description provided.