Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#43857 closed defect (bug) (fixed)

Show the comment / awaiting moderation message even without opt-in

Reported by: imath's profile imath Owned by: pento's profile pento
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords:
Focuses: Cc:

Description

In #43436 a new opt-in checkbox was introduced which is great. But It seems very confusing to me to not have the feedback i used to have before when i've just commented a post: "Your comment is awaiting moderation".

Today when you comment, you have no ideas if the comment was successfully saved. Moreover the Anchor added to the URL in this case leads to nowhere as the comment has not been added to the page.

I think it would be nice to try to preserve this feedback so i'm suggesting the attached patch.

I prefered to open a new ticket about it to avoid bringing some noise to the existing one (#43436), in case you think my approach is a bad idea.

Attachments (12)

43857.patch (7.0 KB) - added by imath 6 years ago.
43857.2.patch (7.1 KB) - added by imath 6 years ago.
Make sure the complete form is cleaned.
43857.3.patch (7.5 KB) - added by imath 6 years ago.
Refresh the patch & make sure potential query vars added by plugins are not altered
43857.4.patch (10.2 KB) - added by imath 6 years ago.
43857.5.patch (10.6 KB) - added by tomdxw 6 years ago.
Refreshed patch ( https://github.com/dxw/WordPress-1/commit/8790643c0a62b8a0806094eb1e622e40a7f33582 )
43857.6.diff (10.6 KB) - added by birgire 6 years ago.
43857.7.diff (10.2 KB) - added by lakenh 5 years ago.
Refreshed patch
43857.8.diff (7.3 KB) - added by imath 5 years ago.
43857.9.diff (4.8 KB) - added by azaozz 5 years ago.
43857.10.diff (5.1 KB) - added by imath 5 years ago.
43857.11.diff (5.5 KB) - added by imath 5 years ago.
43857.12.diff (2.0 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (120)

@imath
6 years ago

#1 @xkon
6 years ago

  • Keywords gdpr 2nd-opinion added

Hey @imath thanks for the info + the patch.

Just to be clear this concerns the redirect done when you did not opt-in for the cookie.

I did a test and I realised this: when you are redirected without opting-in, even though you don't have a cookie (so we're ok there), the form is again pre-filled. On the 2nd 'refresh' or going out & re-entering the page again the comment is vanished of course and the form is clean (we're ok here also).

Having the form prefilled since I'm getting a refresh when I hit the comment button is basically going against the text of the consent as I personally understand it since I skiped the 'save' basically and I'm even more confused now.

Would there be a way of having the form clean and clear as well and just view the comment that has the moderation message? This will surely look more in-line with what we're trying to achieve with the opt-in :) even for just the visual aspect if that makes sense?

I'll add gdpr here as well so we can have it on our list for some extra opinions.

#2 @imath
6 years ago

Hi @xkon

Just to be clear this concerns the redirect done when you did not opt-in for the cookie.

Absolutely, sorry i forgot to mention it in my description :)

Would there be a way of having the form clean and clear as well and just view the comment that has the moderation message?

Sure, i'm adding this need in a new version of the patch (43857.2.patch)

@imath
6 years ago

Make sure the complete form is cleaned.

#3 @xkon
6 years ago

Thanks for that fast update! This looks perfect front-end wise to me now.

This ticket was mentioned in Slack in #gdpr-compliance by xkon. View the logs.


6 years ago

#5 @xkon
6 years ago

  • Summary changed from GDPR compliance shoud not confuse a user who just commented to Show the comment / awaiting moderation message even without opt-in

#6 @xkon
6 years ago

  • Keywords needs-testing added; 2nd-opinion removed

This ticket was mentioned in Slack in #gdpr-compliance by xkon. View the logs.


6 years ago

#8 @desrosj
6 years ago

  • Milestone changed from Awaiting Review to 4.9.6

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

#11 @desrosj
6 years ago

  • Milestone changed from 4.9.6 to 4.9.7

#12 @imath
6 years ago

Hi @desrosj

I usually do not discuss "punting" decisions, I know you and the Core team are doing a great job using a time you could use for something else. I just wanted to let you know although I respect your decision about this ticket i think you took a very wrong one.

4.9.6 is a minor release, yes.. But it will be automatically upgraded for the wide majority of WordPress sites. This means your decision to leave a regression GDPR compliance adjustments introduced into the existing user experience won't be avoidable by these sites.

So if you stay on this line, which given what you consider a "tricky" approach needing test cases about cache consideration is totally understandable, I do advise you to communicate to this wide majority of WordPress sites (eg: at least on make.wordpress.org/core) about the fact that when 4.9.6 will be upgraded to their site : there will be great chances the administrator of these sites will receive similar comments by the same people or questions by people who commented that are unsure their comment was saved etc... because the user did not activate the "cookies consent" checkbox and you decided to postpone this issue to the next minor release.

imho : it's a risky situation because some people might think they have a worse user experience because they didn't consent to store some of their personal data into cookies :(

Anyways, i prefer to inform here I just made a plugin on GitHub in case Administrators want to make sure their users won't have to suffer from this regression until 4.9.7 : https://github.com/imath/gdpr-compliance-is-not-a-worse-user-experience

#13 @desrosj
6 years ago

@imath thank you for the detailed response! I appreciate the feedback and it really helps us prioritize things.

Instead of just moving the ticket, I should have provided proper context when I moved it out of the milestone. We moved this out based on discussion in two consecutive bug scrubs from multiple people that they didn't have the bandwidth to give this one the proper testing.

We decided to punt based on that with the knowledge that it could be added back to the milestone when someone was able to give it a proper review, and that bug fixes can be committed after the beta (which is this Tuesday). Also, we are using 4.9.7 currently for things we are not sure we can get to, but should be next on our list (as opposed to Awaiting Review, Future Release, etc..

Looping in @allendav and @azaozz so that they are aware of your thoughts on this one. Sorry again for not providing context when changing milestone!

#14 @imath
6 years ago

@desrosj many thanks to you for the explanation and it’s ok, I can imagine how it musts be time consuming if you’d explain every punt decisions. No worries.

I still have understanding difficulties about the 4.9.6/4.9.7 dance, but it’s probably because i’m too anxious about this issue i’m considering as a blocker for 4.9.6 ;)

This ticket was mentioned in Slack in #gdpr-compliance by audrasjb. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

#18 @allendav
6 years ago

At this late stage (of 4.9.6), I think we should just

1) add a filter to disable the 43436 behavior
and
2) also fix the persistent form-fill-in that @xkon notes in the first comment on this ticket

Will that help @imath ? It should give us time to come up with a robust solution for moderation without opt-in

#19 @imath
6 years ago

@allendav Thanks for looking at this bug. I really appreciate your consideration.

FYI 43857.patch was leaving the form filled for next comments, and 43857.2.patch is fixing that form-fill-in.

Then, sorry but I'm not sure to understand your plan for this late stage. If I rephrase does this mean :

  1. Introduce a filter that would prevent the opt-in checkbox about cookies consent to be displayed on the comment form
  2. Store cookies to make sure the awaiting moderation message is still displayed to the user so that he sees his comment was saved but make sure the form is emptied for next comments ?

I think 1. can already be achieved filtering comment_form_default_fields and 2. seems a weird behavior to me because it looks like hiding cookies are saved so i'm probably misunderstanding.

Options i see :

  1. If losing the information "your comment is awaiting moderation" is not a regression for comment authors, then fine, let's just remove the anchor to the comment that is appended to the redirect URL when ! $cookies_consent. Because it leads to nowhere in the comments list of the loaded page.
  1. If losing the information "your comment is awaiting moderation" is a regression for comment authors :
  • the comment ID needs to be added as a query var to the URL so that it's possible to include the comment at next page load (for example the attached 43857.2.patch).
  • or you can wp_die( '<p>Your comment is awaiting moderation</p>', 'Comment awaiting moderation', array( 'back_link' => true ) );

@imath
6 years ago

Refresh the patch & make sure potential query vars added by plugins are not altered

#20 @desrosj
6 years ago

  • Component changed from Comments to Privacy

Moving to the new Privacy component.

#21 @desrosj
6 years ago

  • Version changed from trunk to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#22 @lakenh
6 years ago

#44160 was marked as a duplicate.

This ticket was mentioned in Slack in #core by lakenh. View the logs.


6 years ago

#24 @desrosj
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#25 @jeremyclarke
6 years ago

FWIW I want to +1 the view that this is a tremendous problem that should have been a blocker for the update. It fails silently in a very bad way and in a way that is very hard to test. Only dev teams doing very detailed QA will have noticed this problem, especially because it won't affect any site admins (who are of course logged in).

On top of this, many themes over the years use other methods to generate comment forms, in which case the consent checkbox doesn't show at all. In that scenario this situation is even worse, because the cookie is never generated and no one sees the moderation message.

If the site I manage had auto-updated without my intervention and testing first, our commenting would have become incoherent because this patch wasn't included. In a point release.

---

Other thought: Having this work properly by default (moderation message always shows regardless of cookie) is very important because many sites should just choose to ditch the cookie and it's consent woes entirely.

It's really not a big deal to fill out comments again, and honestly I'd rather have the form and privacy policy be simpler. It strikes me that if this cookie-based feature didn't already exist in core, it would never get added now that GDPR makes it such a headache for everyone involved.

Just looking at #43436 and all the important information @johnjamesjacoby brought up that should be shown with the checkbox, but isn't for usability reasons, really drives this home.

As site admins, opting out of the comment cookie entirely is the killer feature. Excited for this patch to be added ASAP.

#26 @superpoincare
6 years ago

Also, I want to raise this point:

Let's say I use Twenty Seventeen, and uncheck "Comment author must fill out name and email" in Discussion settings.

Then some visitor to my site comments and only enters the name but not the email or maybe neither. Then what?

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#28 @desrosj
6 years ago

  • Keywords needs-refresh added

Patch is no longer applying cleanly.

#29 follow-up: @imath
6 years ago

@desrosj no problem, I'll update it as soon as 4.9.8 dev cycle starts as it's scheduled for this specific release, no emergency or am I wrong ?

#30 @desrosj
6 years ago

@imath No emergency. In the Privacy bug scrub today this was referenced while discussing #44373 (https://wordpress.slack.com/archives/C9695RJBW/p1529937786000362).

Since we are early on in the 4.9.7, this could be moved back. This was moved to 4.9.8 in an effort to clean out the very overloaded (at the time) 4.9.7 milestone. Tickets that are ready to land can definitely be moved back. @azaozz is going to keep this one in mind while checking out #44373.

#31 @jerclarke
6 years ago

Please move this one to 4.9.7 if at all possible! The effect on sites with moderation is awful.

#32 @mdawaffe
6 years ago

Just dropping a note here that the current patch allows anyone to enumerate and view all unapproved comments (that have a non-empty comment_author_email) by adding ?unapproved=1, ?unapproved=2, ….

I agree that the current behavior is frustrating.

#33 in reply to: ↑ 29 @pbiron
6 years ago

Replying to imath:

@desrosj no problem, I'll update it as soon as 4.9.8 dev cycle starts as it's scheduled for this specific release, no emergency or am I wrong ?

We're starting that "soon", so I'd be great it you could refresh the patch in the next week or so.

@imath
6 years ago

#34 @imath
6 years ago

  • Keywords needs-refresh removed

Hi @pbiron

Thanks for your update. 43857.4.patch is the refreshed the patch.

Well to take in account @mdawaffe 's comment i've added a moderation hash.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#36 @desrosj
6 years ago

  • Keywords gdpr removed

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

#37 @pbiron
6 years ago

  • Keywords changed from has-patch, needs-testing to has-patch needs-testing

Can anyone test 43857.4.patch?

4.9.8 beta is scheduled for July 17 (next Tues) and this could land if gets some testing.

This ticket was mentioned in Slack in #core by pbiron. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#40 @desrosj
6 years ago

  • Milestone changed from 4.9.8 to 4.9.9

4.9.8 RC has been released. Moving to 4.9.9.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#42 @desrosj
6 years ago

  • Keywords needs-refresh added

43857.4.patch needs to be refresh to apply cleanly.

#43 @tomdxw
6 years ago

  • Keywords needs-refresh removed

@desrosj Any chance we can get this into 4.9.9?

#44 @superpoincare
6 years ago

I used the beta-tester plugin with bleeding edge nightlies and in the version alpha-43581 it is not working.

This is the test site provided by WP Sandbox. Since it's free it's only for 1 day. I just set it up. Someone can test it there:

https://bit.ly/2weYlNf

Version 0, edited 6 years ago by superpoincare (next)

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#46 @desrosj
6 years ago

  • Owner set to javorszky
  • Status changed from new to assigned

@superpoincare the changes being suggested in this ticket so far have not yet been applied to WordPress core. To test, please see the Testing a Patch section of the handbook.

#47 follow-up: @robdxw
6 years ago

@javorsky Is there anything I can do to help get this in 4.9.9? Unit tests are all passing, and I've done some local testing which all seems ok. But it's not clear from https://make.wordpress.org/core/handbook/testing/patch/ whether that's sufficient, or if anything else is required.

#48 in reply to: ↑ 47 @javorszky
6 years ago

  • Keywords needs-refresh added

My username (which is my last name) is missing a 'z'. :)

Things I'm missing at the moment:

  • refreshed patch - using the docs here: https://make.wordpress.org/core/handbook/testing/patch/, applying the patch via the grunt way fails to apply 2 of 3 hunks, so it doesn't seem like I can test this in any meaningful way as of this moment
  • as someone who's new to testing this part of the code, I need a few steps to set up the opening state of things. What settings should WordPress have, how to have a user submit a comment without consent, etc?
  • then a way to see what this patch should be solving. For example: "If (after common setup steps) user does X, without this patch, this happens. With the patch, this other thing happens". Gifs and screenshots help.

Replying to robdxw:

@javorsky Is there anything I can do to help get this in 4.9.9? Unit tests are all passing, and I've done some local testing which all seems ok. But it's not clear from https://make.wordpress.org/core/handbook/testing/patch/ whether that's sufficient, or if anything else is required.

#49 @superpoincare
6 years ago

Hi @javorszky

Settings:

  • Under Settings > Discussion > Other comment settings", choose: "Show comments cookies opt-in checkbox.".
  • Under Settings > Discussion > Before a comment appears, choose both "Comment must be manually approved" and "Comment author must have a previously approved comment".

Without patch:

Comment made by a general user won't show up on his/her browser after he/she submits the comment if "Save my name, email, and website in this browser for the next time I comment." is not chosen. Only after approval.

With patch:

Comment made in the same way should appear once (assuming patch works) after the page refreshes and the browser fetches the comment. Won't appear on subsequent visits till approval.

This ticket was mentioned in Slack in #core-privacy by javorszky. View the logs.


6 years ago

@birgire
6 years ago

#51 @birgire
6 years ago

  • Keywords needs-refresh removed

43857.6.diff is a refresh of 43857.5.patch that currenlty doesn't applies cleanly.

Also updates the since annotation to @since 4.9.9.

#52 @javorszky
6 years ago

  • Keywords needs-testing removed

Thanks for the info for testing.

Code looks okay, functionality also works as described:

  • without this patch, when submitting a comment by a logged out person will not tell the person that the comment was received
  • with this patch the logged out user is told once immediately after submitting

I was thinking whether telling the person who submitted a comment that their comment is held in moderation a necessary part of the site functioning (and therefore should not be subject to consent), but multiple people using the same computer complicates that.

I think this is ready to go in as-is. :)

#53 @superpoincare
6 years ago

Hi @javorszky,

Yeah had raised this point about telling the user here: https://core.trac.wordpress.org/ticket/43436#comment:69

But maybe another ticket once this goes, as you say.

#54 @idea15
6 years ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-privacy by javorszky. View the logs.


6 years ago

#56 @javorszky
6 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

Office hours:

To go around the issue of disappearing "held in moderation" notice @tz-media suggested we expand the checkbox's text to warn that without it the notice will only be visible once.

We might show a warning on the actual comment cookies opt in, but I wouldn't have a second opt in for that.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#58 @mallorydxw
6 years ago

Do any changes need to be made to the patch, aside from refreshing it?

Is there anything else that's needed to move this ticket along?

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#60 @pento
6 years ago

  • Milestone changed from 4.9.9 to 5.0.1

#61 @SergeyBiryukov
6 years ago

#45090 was marked as a duplicate.

#62 @aparentdesign
6 years ago

While we are awaiting the release of 5.0.1, is there a recommended workaround?

#63 @superpoincare
6 years ago

@aparentdesign

Yeah, this plugin: https://github.com/imath/gdpr-compliance-is-not-a-worse-user-experience

Made by imath, the issue creator and fixer.

#64 @desrosj
6 years ago

  • Owner javorszky deleted

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


6 years ago

#66 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#67 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 years ago

#69 @desrosj
5 years ago

  • Milestone changed from 5.0.3 to 5.1

Still needs a refresh and 5.0.3 RC is today. Punting to 5.1.

@lakenh
5 years ago

Refreshed patch

#70 @lakenh
5 years ago

  • Keywords needs-refresh removed

I would really like to see this get merged soon, as it still is a rather serious regression in my opinion that was introduced in 4.9.6.

This ticket was mentioned in Slack in #core-privacy by lakenh. View the logs.


5 years ago

#72 @imath
5 years ago

Hi everyone,

What has to be done to have this in ? Unit test ?

We're approaching 5.1, you released 4.9.6 knowing this regression was in and you're punting the ticket from version to version. Let's *really* try to fix this once and for all please.

#73 @pento
5 years ago

  • Owner set to pento

#74 @pento
5 years ago

  • Keywords needs-refresh added
  • Owner pento deleted

I've tested 43857.7.diff a bit, here are the issues I ran into:

  • When permalinks are set to Plain, the URL after posting a comment looks like this: http://localhost:9999/?p=169?p=169#comment-16
  • The "Your comment is awaiting moderation." message should be updated to note that they'll only see this message once.

Generally, I have a few concerns about the patch itself:

  • wp_remove_feedback_query_args() seems like overkill. Is there a benefit to remove the hash from the URL, apart from looking nice?
  • The logic in wp_get_current_commenter() is confusing. It seems to setting a current commenter so that wp_list_comments() will load the appropriate comment, but it's also setting a flag so that comment_form() doesn't show those commenter details in the comment form after submitting a comment? It seems like this bit of logic to detect the commenter should really only happen in wp_list_comments(), as that's the only place we want to know about this commenter.
  • There are no unit tests. See commentForm.php and commentsTemplate.php for examples.

And some minor issues:

  • There are a handful of typos and code formatting issues. Please run composer run format on the changed files, to ensure they're correct.
  • When comparing the hash, please use hash_equals() rather than ===.

I'm happy to leave this ticket in 5.1 for now.

#75 @superpoincare
5 years ago

@pento,

I think the query args with a hash are added first so that nobody outside can find unapproved comments. But once the comment is approved, the comment URL should look like other comment URLs.

Last edited 5 years ago by superpoincare (previous) (diff)

#76 @imath
5 years ago

Hi @pento

Thanks a lot for your feedback, review and inputs about the unit tests. I'm working on a new version of the patch to take all this in account. I'll update the ticket asap.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

#79 @pento
5 years ago

@imath: How's the patch going? Beta 2 is on Monday, which is the deadline for committing bug fixes that aren't 5.1 regressions.

#80 @imath
5 years ago

Hi @pento I think about it every day !! Unfortunately I had no time to work on it so far (Job + WordCamp Paris needed too much of my time). I’ll have it updated before Sunday. I promise.

#81 @hatesspam
5 years ago

What is the equivalent 4.9 issue?

#82 @azaozz
5 years ago

To add to @pento's remarks above:

  • This patch should improve the functionality of showing unapproved comments to the commenter right after a comment is posted. Cookies should not be used any more for that.
  • It should not deal with (or touch) cookies or cookies consent. Comment cookies become completely different feature.

Looking at 43857.7.diff, all parts dealing with cookies should be removed.

Also, the canonical URL doesn't need to include any query args used to implement this functionality. Seems best to remove them in the links template. If we do that we would also need to "fix" the URL displayed in the browser, i.e. do the window.history.replaceState() from js.

@imath
5 years ago

#83 @imath
5 years ago

  • Keywords needs-refresh removed

Hi @pento

43857.8.diff is the new patch that followes your recommandations.

  1. I was not able to reproduce the issue you've found when permalinks are plain. I get something like this: http://site.url/?p=1&unapproved=7&moderation-hash=a80413952f193db0cb127ce2ebaf7fd2#comment-7
  1. The message "Your comment is awaiting moderation." is output into Walker_Comment, if I added a complementary message as you requested, there's not guarantee it will be output to the user as themes can override this class.

Most "teen" themes are not overriding the class but TwentyNineteen does, should I also update TwentyNineteen_Walker_Comment to make sure the specific message is also output ?

  1. About wp_remove_feedback_query_args() I removed all the code as I was using it to be sure the URL looked the same as before 4.9.6.
  1. About wp_get_current_commenter() I removed all the code and created a new function wp_get_unapproved_commenter_email() as there are two functions where we need to check for the current commenter :
  • wp_list_comments()
  • comments_template()
  1. I've added a unit test named test_comments_list_should_include_just_posted_unapproved_comment()
  1. Code formatting issues, I ran composer run format on all edited files.
  1. About typos or more globally english text: as I'm not english, if there are still a handful of typos, could you or someone who's english tell me where they are and what is the fix ?
  1. The patch is now using hash_equals() to compare the hash.

I'm happy to improve the patch if needed.

#84 @azaozz
5 years ago

Replying to imath:

Seems we've been looking at this at the same time :)

43857.8.diff looks a lot better. The only thing remaining would be to remove use of comment cookies for this functionality completely, consent or no consent.

Also, to avoid dealing with canonical URLs it may be possible to use the hash part of the URL when redirecting. Then the (redirect) URL would look something like https://site.tld/post-with-comments/#comment-1234_abcd1234 where abcd1234 would be the WP hash allowing the user to see their unapproved comment. Then we can use window.history.replaceState() to remove the _abcd1234 part.

As far as I see this should work, the browser will scroll to the location of the comment, if that script runs in the head. Will test a bit more.

Last edited 5 years ago by azaozz (previous) (diff)

#85 @azaozz
5 years ago

Hmm, thinking more about it, can't use the hash part of the URL here as the page will probably be cached by plugins that do static front-end caching. Using query args should prevent that.

@azaozz
5 years ago

#86 @azaozz
5 years ago

43857.9.diff is based on 43857.8.diff but doesn't use comment cookies to show comments awaiting moderation. Also includes some improvements and fixes few issues:

  • Disable when email is not present (emails may not be required for posting comments).
  • Prevent warning when query args are invalid.
  • Also add the same functionality to wp_list_comments().

#87 @superpoincare
5 years ago

@azaozz

I tested your diff.

Your diff forgot to remove one line

esc_url( add_query_arg( 'replytocom', $comment->comment_ID ) ) . '#' . $args['respond_id'],

in comment-template.php

That makes its appearance as "Reply" below a comment becomes "Reply to [Commenter Name]" and "Leave a Reply" above the comment box becomes "Leave a Reply to [Commenter Name]"

Edit:

Further, if I remove that line, clicking "Reply" below any approved comment, refreshes the page and takes it to the server. That shouldn't be happening. Also, "Reply" shows as "Reply" but "Leave a Reply" shows as "Leave a Reply to [commenter name]"

Last edited 5 years ago by superpoincare (previous) (diff)

@imath
5 years ago

#88 @imath
5 years ago

Hi @azaozz

Thanks a lot for your improvements, I totally agree with your opinion about :

remove use of comment cookies for this functionality completely, consent or no consent

I've tested the patch and I confirm what @superpoincare found (good catch by the way).

So in 43857.10.diff​ I removed the line. One weird thing I ran into was that it was added back when I did composer run format src/wp-includes/comment-template.php So I removed it again :)

I also removed a remaining inline comment that was saying to users who refused to use comment cookies.

#89 @superpoincare
5 years ago

There's a ticket which makes changes to comment replies, which I had ignored in my previous tests.

https://github.com/WordPress/WordPress/commit/f617a2d6d932e658449cef1b7864fb77080ecd09#diff-3db08a6b98155b349c93a220bc903dce

That changes the file comment-reply.min.js which takes care of clicking on replies.

That should stop reloading the page. I think "Cancel reply" (it's a link) may have some issues.

Last edited 5 years ago by superpoincare (previous) (diff)

#90 @superpoincare
5 years ago

A more accurate description of my testing compared to my previous comment.

The fix works, (assuming new wp-includes/js/comment-reply.min.js of #31590 is present).

Clicking "Reply" to an approved or unapproved (your own, if any) doesn't refresh the page, as should be.

"Cancel reply" also works, but the cancel reply is a link and its link has the moderation hash. For the sake of clarity, it may need to be removed, although not strictly necessary.

If it's decide, the function get_cancel_comment_reply_link in comment-template.php needs to be modified.

@imath
5 years ago

#91 @imath
5 years ago

In 43857.11.diff I make sure to remove the extra query arguments from the cancel comment reply link.

Thanks for the catch @superpoincare 👍

#92 @pento
5 years ago

  • Owner set to pento
  • Status changed from assigned to accepted

#93 follow-up: @pento
5 years ago

Removing the cookie support is incorrect. If cookies are enabled, the commenter should be able to revisit the site and see their unmoderated comments. The test_query_offset_should_include_unapproved_comments test exists to show this behaviour.

I'll add cookie support to wp_get_unapproved_comment_author_email().

#94 @pento
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 44659:

Comments: Show the "awaiting moderation" message when comment cookies are disabled.

The "Your comment is awaiting moderation." message relied upon the comment author cookie being set. However, since it's now possible to opt-out of that cookie, submitting a comment won't show the comment preview when the comment is placed in moderation.

To avoid this issue, we now include a hash in the redirect URL, allowing the site to identify that a preview of the moderated comment should be displayed.

Props imath, tomdxw, birgire, lakenh, azaozz, pento.
Fixes #43857.

#95 in reply to: ↑ 93 ; follow-up: @azaozz
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to pento:

Removing the cookie support is incorrect. If cookies are enabled, the commenter should be able to revisit the site and see their unmoderated comments.

Why? What is the value for the user going to an old page and seeing their unapproved comments only on some sites?

This introduces a pretty annoying inconsistency and seems pretty damaging for the user experience: some sites will "tell" the commenter that the admin still hasn't approved their comments from a month ago, other will not (i.e. the commenter would assume their comments were rejected/deleted). Thinking that is wrong unless there is an explanation printed under all comment forms that explains how this works.

Last edited 5 years ago by azaozz (previous) (diff)

#96 in reply to: ↑ 95 @pento
5 years ago

Replying to azaozz:

Why? What is the value for the user going to an old page and seeing their unapproved comments only on some sites?

That's how it's worked since forever.

I'd be fine with changing that, but it's a bigger task than this ticket, which was just about making existing behaviour (or a reasonable approximation thereof) work without cookies.

#97 follow-ups: @azaozz
5 years ago

That's how it's worked since forever.

Right, but it doesn't any more (thanks to compliance with web regulations) :)

making existing behaviour (or a reasonable approximation thereof) work without cookies.

Thinking that introducing the inconsistency is a lot worse than not fixing this at all. Imagine what would happen if one site on wordpress.org/make had support for cookies so users can see their unmoderated comments for months, and another did not, so users wrongly perceive their unmoderated comments were deleted... :)

Not getting any feedback after posting a comment held for moderation is bad, thinking your comment was deleted is... a lot worse :)

Last edited 5 years ago by azaozz (previous) (diff)

#98 @superpoincare
5 years ago

@azaozz

The best thing IMO would be to add a note below "Save my name, email, and website in this browser for the next time I comment." telling user of the behaviour - differences between checking the checkbox and not.

I have this:

Note: If the checkbox above is unchecked and you click the submit button below, and if the comment goes into moderation, your comment may appear only once in your browser, i.e., after the page refreshes, with a message: Your comment is awaiting moderation., not on subsequent visits till it is approved.

on my site.

#99 in reply to: ↑ 97 @hatesspam
5 years ago

Replying to azaozz:

Imagine what would happen if one site on Wordpress.org/make had support for cookies so
users can see their unmoderated comments for months, and another did not, so users
wrongly perceive their unmoderated comments were deleted... :)

People do not visit Wordpress sites, they visit websites. And across all websites, comment section behaviour is going to differ anyway.

#100 in reply to: ↑ 97 @pento
5 years ago

Replying to azaozz:

That's how it's worked since forever.

Right, but it doesn't any more (thanks to compliance with web regulations) :)

"thanks to [some interpretations of requirements for] compliance with [regional] web regulations". 😉

If you don't use cookies, it's a reasonable expectation that the site won't remember who you are, so can't show you your "in moderation" comments.

The easiest fix seems to be to default show_comments_cookies_opt_in to being turned on, which we can do in #44736.

#101 @azaozz
5 years ago

@superpoincare right, there was an additional message shown when not using cookies in 43857.8.diff.

across all websites, comment section behaviour is going to differ anyway.

True, we can only try to do "the best" for visitors of WP sites. Considering that's about 1/3 of the more popular sites on the internet, chances are most other sites/software will adopt the same or similar behavior (if we get it right of course) :)

"thanks to [some interpretations of requirements for] compliance with [regional] web regulations". 😉

Hehehhhehe, true :) I really hope we won't need to add some sort of "compliance analysis" in WP warning the site owners when some settings don't seem "compliant" with that we think is the location of the site. Looks like it may need something like that in not-so-distant future...

The easiest fix seems to be to default show_comments_cookies_opt_in to being turned on, which we can do in #44736.

Sure, lets do that.

Also thinking it would be better to add something like the extra text in 43857.8.diff. That will remove the chance of misunderstandings which might be really bad in some cases for both the commenters and the sites they posted comments on.

That text extends the message shown when a comment is held for moderation and there are no cookies. We can indicate that the comment won't be visible until approved. Perhaps something like:

Your comment is awaiting moderation. (with cookies).

Your comment is awaiting moderation. It is visible only to you and only after it is posted. (without cookies).

Last edited 5 years ago by azaozz (previous) (diff)

#102 @pento
5 years ago

  • Keywords has-patch removed

Yah, adding some extra text works for me.

@azaozz
5 years ago

#103 @azaozz
5 years ago

In 43857.12.diff: add (back) some more text explaining that when no comment cookies or no email, the preview of a comment awaiting moderation is shown only after posting the comment.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


5 years ago

#105 @pento
5 years ago

Fixed in [44681].

#106 @pento
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#107 @superpoincare
5 years ago

Twenty Nineteen has its own class-walker-comment file 😁

https://github.com/WordPress/WordPress/blob/master/wp-content/themes/twentynineteen/classes/class-twentynineteen-walker-comment.php

But I am not sure what what Changeset 44681 does. I am not able to get it to see the new moderation message though in various scenarios/settings in my tests.

Changeset 44659 works, of course.

#108 @SergeyBiryukov
5 years ago

In 46117:

Bundled Themes: Update the message shown when a comment is awaiting moderation.

If the commenter doesn't have cookies set, they won't see the comment preview again. Showing an expanded message will help offset any confusion if they revisit the site later, and their comment is still in moderation, but they can't see it anymore.

Props birgire, superpoincare.
Fixes #47461. See #43857.

Note: See TracTickets for help on using tickets.