fix(comments): keep comment access_id in sync with container #8062

beck24 · 2015-03-13T08:46:22Z

Fixes #7807, Fixes #7984

Voting time

Yes let's pull this in:

Matt
Evan
Brett
Juho

Yes with more:

Steve (show UI message when expanding access, don't close WSOD when searching for "event calendar" on community site #7984)

No let's do something else:

beck24 · 2015-03-13T08:55:55Z

Tested this manually by doing the following:

installing 1.x
create a blog post -> access public
create 2 comments -> check db -> they are access public
change blog post -> access logged in
check db -> comments still access public
switch to this branch -> run upgrade script
check db -> comments access logged in
run unit tests -> all pass
repeat steps 2-5 - comment access now syncs

beck24 · 2015-03-13T08:57:47Z

Travis makes me sad :(
Anyone know what's up with that?

hypeJunction · 2015-03-13T09:00:20Z

engine/classes/ElggEntity.php

@@ -1686,7 +1686,14 @@ protected function update() {
 		unset($persisted_entity);

 		if ($allow_edit) {
-			$allow_edit = _elgg_services()->events->trigger('update', $this->type, $this);
+			$allow_edit = elgg_trigger_before_event('update', $this->type, $this);


Why not use _elgg_services()?

Not sure these changes belong in this commit. It will be a major pain point for plugin upgrades and should have it's own entry in the logs.

I thought the before/after functions were there for just this reason

This doesn't really work without the after hook, as a plugin/something could prevent the update, but then the comments would get changed to a different access_id - making the situation worse.

It will be a major pain point for plugin upgrades
I wouldn't call it a major pain point - append ':before' or ':after' to the registration event. Minor inconvenience at best :)

Having it's own entry in the logs is a decent point though.

So much for less intrusive deprecation behaviour ;) I don't think any of the older events would need to use :before, so maybe there is a way to make this work without deprecation warning.

Technically all this needs to work is the after event. I could leave the old update event in place, it just seemed logical that if I was adding the after event it should have a matching before event

I vaguely remember a good reason I didn't migrate entity "update" to before/after events, and I don't recall what it was!

@beck24 I think leaving "update" as is and adding "update:after" is an OK compromise. We'd need to mention it in the RST docs

ok, I'll remove the deprecation and leave 'update' as is with the added 'update:after'

hypeJunction · 2015-03-13T09:25:07Z

I am wondering if we should create a new method (maybe ElggEntity or its own service) that will take $type, $subtype as arguments and sync access of all contained entities of a given subtype to that of an entity. The reason I am suggesting this is that discussion_update_reply_access_ids uses the same logic, and I believe plugins are using these batch operations as well.

hypeJunction · 2015-03-13T09:27:16Z

Travis is complaining about lint:

FILE: /home/travis/build/Elgg/Elgg/engine/lib/comments.php

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)

301 | ERROR | Missing @return tag in function comment

beck24 · 2015-03-13T10:01:37Z

I was thinking the same thing about a generic method to handle other types, wasn't sure about the best way to go about it and I wanted this fixed asap.

juho-jaakkola · 2015-03-13T12:26:04Z

engine/lib/comments.php

+			'wheres' => array(
+				"e.access_id != {$entity->access_id}"
+			),
+			'limit' => 0,


I don't know about any convention, but I always use false.

I looked at what the discussions batch for this did and it used 0, but I do prefer false

ewinslow · 2015-03-13T12:40:36Z

I wish this did not have to be an upgrade and instead was a computed property. This is bound to go out of sync again some time in someone's install because of some bad plugin or bug, no?

beck24 · 2015-03-13T16:05:16Z

The only way I can see it getting out of sync after this is if someone deliberately unregisters the event handler, or prevents the comment update with an event handler. Or a database interruption I suppose, but that's a risk on any operation.

After the upgrade is complete we could occasionally run a global check/fix on cron.

mrclay · 2015-03-13T18:05:12Z

actions/admin/upgrades/upgrade_comments_access.php

+$access_status = access_get_show_hidden_status();
+access_show_hidden_entities(true);
+
+// don't want any event or plugin hook handlers from plugins to run


If we're going to disable all this stuff, wouldn't raw queries be a lot faster?

mrclay · 2015-03-14T03:26:05Z

The access_id also needs to inherit into likes on the comments, so we probably have to call events, right?

beck24 · 2015-03-14T03:27:20Z

Yes, I have the event calls in the most recent iteration here

mrclay · 2015-03-14T20:05:02Z

actions/admin/upgrades/upgrade_comments_access.php

+	$comments = elgg_get_entities($options);
+
+	foreach ($comments as $comment) {
+		$container = $comment->getContainerEntity();


Maybe check the container is actually loaded?

ok, will do. This is run with hidden status and access overridden, so there shouldn't be any case where the container can't be loaded unless there's a data problem where the container is missing or broken. In that case what do you think we should do with it?

Just decided to skip it and record it as an error. There's no good way to predict what should happen. Devs will have to sort out their own data corruption.

Fixes #7807, Fixes #7984

socialatm · 2015-03-15T14:14:38Z

If someone makes a comment based on the entity is access_friends and then the entity gets changed to access_public how will that work? Will the comment still be access_friends?

I guess my concern is that I don't want the comment to be less restrictive than the owner intended it to be...

beck24 · 2015-03-15T18:24:54Z

That's how it currently works, but with this the comment will change to be access_public - it will be kept in sync with the entity.

socialatm · 2015-03-15T19:02:33Z

So in this scenario if you commented on my post and then I change the access in effect I'm changing the access permission on a comment I don't own? I see the issue as it's not okay for a comment to be less restrictive than the entity but shouldn't it be okay for the comment to be more restrictive than the entity? I'm just looking to honor the privacy settings of the comment owner. Thanks...

for example: I make a comment to my friends about my job and then the access gets changed and now my boss can see it?

hypeJunction · 2015-03-15T19:22:43Z

IMHO, comment owner should not even be able to control the privacy of a
comment. Comment threads would make no sense if you omitted half the
comments due to their privacy settings.
On Mar 15, 2015 8:02 PM, "Ray" notifications@github.com wrote:

So in this scenario if you commented on my post and then I change the
access if effect I'm changing the access permission on a comment I don't
own? I see the issue as it's not okay for a comment to be less restrictive
than the entity but shouldn't it be okay for the comment to be more
restrictive than the entity? I'm just looking to honor the privacy settings
of the comment owner. Thanks...

—
Reply to this email directly or view it on GitHub
#8062 (comment).

socialatm · 2015-03-15T19:54:54Z

@hypeJunction
the comment is an entity so the entity owner should not have control over the privacy setting?
IMHO it's okay to make the comment more restrictive but it's not okay to make it less restrictive.
I'm just trying to look at it from the user point of view

hypeJunction · 2015-03-17T10:51:10Z

As was told earlier, if you are dealing with sensitive information,
unregister the hook and implement your own logic. We are talking about
comments here not medical records.
On Mar 17, 2015 11:49 AM, "Ismayil Khayredinov" info@hypejunction.com
wrote:

I am all up for other solutions. So far I only heard this doesn't work for
me - what will then? Syncing seems like the simplest solution. If anyone
can submit an alternative PR I am sure we will gladly consider it. And this
is only a temporary solution until the collections API is in place.
On Mar 17, 2015 10:47 AM, "Jerôme Bakker" notifications@github.com
wrote:

Why not have an admin delete the post and have the poster write a new one?

what if that is not allowed by company policy?

—
Reply to this email directly or view it on GitHub
#8062 (comment).

jeabakker · 2015-03-17T10:52:18Z

So far I only heard this doesn't work for me - what will then?

i just made a fix for a client of our where i just catch the fact that the container (where the comment is on) is not accessable and make a different text.

Will submit pr soon

mrclay · 2015-03-17T10:52:55Z

The admin could communicate with the user not through comments. Point being this is an extremely narrowly constructed hypothetical situation so counter arguments can weasel out of it.

We should probably discuss on the community what the most common cases are and the behavior least surprising to most users. The answer may be syncing in some cases and not others.

socialatm · 2015-03-17T10:58:17Z

I never did get the question answered "do you want some website changing your privacy settings?". But as long as I have an unregister hook I'll deal with it.

mrclay · 2015-03-17T11:14:27Z

As I mentioned in an older ticket, from the perspective of the commentor's privacy, not altering the comment's access level is a feature. I think the desires of the content author to change access level have to come second.

ewinslow · 2015-03-17T18:14:46Z

@mrclay what about a UI warning?

Also consider the case of email -- you can forward a whole conversation to new people, or add people to the conversation and have them see the whole history.

My opinion is that a UI warning about the social hazards of changing access levels would be a good compromise.

beck24 · 2015-03-17T18:17:55Z

@jeabakker This has never been the case in Elgg, why start now?

This currently happens for replies to group discussions for the same reasons.

ewinslow · 2015-03-17T18:18:49Z

Historical precedent doesn't hold much weight for me in general. Lots of things can be done wrong for a long time...

hypeJunction · 2015-03-17T18:22:40Z

Can I suggest that elgg owners take a vote to either pull this in or close?
It's too much blah blah.
On Mar 17, 2015 7:18 PM, "Evan Winslow" notifications@github.com wrote:

Historical precedent doesn't hold much weight for me in general. Lots of
things can be done wrong for a long time...

—
Reply to this email directly or view it on GitHub
#8062 (comment).

socialatm · 2015-03-17T21:39:46Z

I think a better way to go is if the logged in user can not see the entity then don't show the comments either. And still no one has answered the question "do you want some website changing your privacy settings?".

I'm a privacy advocate, I just am.

beck24 · 2015-03-17T21:46:23Z

I think a better way to go is if the logged in user can not see the entity then don't show the comments either.
That's exactly what this PR will do.

do you want some website changing your privacy settings
You are exaggerating the issue, this isn't changing someones privacy settings. If the person doesn't choose a privacy setting when they're creating the comment, it's not changing a setting for them.

socialatm · 2015-03-17T21:50:50Z

@beck24 You are correct on "that's exactly what this PR will do" as long as the entity owner is making the access_id more restrictive but not if they're making it less restrictive.
I'm new so maybe I'm missing something? A user does choose to comment or not based on the privacy setting of the entity.

Was that a yes or no to having a website changing your privacy settings?

ewinslow · 2015-03-17T21:54:29Z

@twentyfiveautumn That was a no.

ewinslow · 2015-03-17T21:54:46Z

Agreed that we need a vote.

socialatm · 2015-03-17T21:57:15Z

+1 on a vote

beck24 · 2015-03-17T21:59:07Z

Agreed - vote initiated at the top of the thread

beck24 · 2015-03-18T02:41:38Z

@mrclay - just saw your vote. I'm fine with both of those conditions - would you want the UI message added to this PR?
Next is the question of how we define expanding access. That could be tricky to decide when you consider custom acls, probably requiring an ajax callback to do an array diff of acl membership. It could be useful to have a notice when restricting the access as well. Maybe something generic-but-informative on any change?

mrclay · 2015-03-18T03:37:47Z

Same PR seems fine; up to you. A generic message that appears on any access_id select change seems sufficient.

socialatm · 2015-03-18T11:09:18Z

This feels like a backdoor but letting the user know it's there is certainly a good step in the right direction. Thank you for that. My intention here has always been to look out for the interest of all users on all Elgg websites. Thanks again
Ray

socialatm · 2015-03-19T11:23:51Z

When you strip away the debate and opinions here the actual issue is simple:
The code allows a user to change the access_id of an entity they do not own.
Some see no harm in that, others do.
That's the entire issue.
I do apologize to anyone I've offended along the way.
Ray

mrclay · 2015-03-19T12:26:47Z

@twentyfiveautumn I may own that entity, but it's an Elgg system implementation detail that ElggComments have independent access control and ownership. If I comment on someone else's WordPress blog, they retain some rights to moderate my comment in various ways and I see that as reasonable.

I think contextual help to explain features/site policies to users would be helpful but ultimately the demand from developers hasn't been that great for it.

mrclay · 2015-03-19T12:28:45Z

Sorry everyone, we can move our discussion to the community.

beck24 · 2015-03-20T07:57:16Z

I've created a new ticket for the UI feedback as this is ready to merge and I think we're all done with this thread!

fix(comments): keep comment access_id in sync with container

Srokap added the in progress label Mar 13, 2015

hypeJunction reviewed Mar 13, 2015
View reviewed changes

juho-jaakkola reviewed Mar 13, 2015
View reviewed changes

mrclay reviewed Mar 13, 2015
View reviewed changes

mrclay reviewed Mar 14, 2015
View reviewed changes

fix(comments): keep comment access_id in sync with container

066102a

Fixes #7807, Fixes #7984

beck24 mentioned this pull request Mar 20, 2015

UI feedback when changing access on entities with comments #8086

Closed

beck24 added a commit that referenced this pull request Mar 20, 2015

Merge pull request #8062 from beck24/7807-comment-access

4dfc956

fix(comments): keep comment access_id in sync with container

beck24 merged commit 4dfc956 into Elgg:1.x Mar 20, 2015

Srokap removed the in progress label Mar 20, 2015

beck24 mentioned this pull request Mar 20, 2015

Keep comment access_id in sync with container #7807

Closed

jdalsem mentioned this pull request Mar 24, 2015

WSOD when searching for "event calendar" on community site #7984

Closed

beck24 deleted the 7807-comment-access branch March 30, 2015 06:10

fix(comments): keep comment access_id in sync with container #8062

fix(comments): keep comment access_id in sync with container #8062

Conversation

beck24 commented Mar 13, 2015

Voting time

beck24 commented Mar 13, 2015

beck24 commented Mar 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypeJunction commented Mar 13, 2015

hypeJunction commented Mar 13, 2015

FILE: /home/travis/build/Elgg/Elgg/engine/lib/comments.php

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)

beck24 commented Mar 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewinslow commented Mar 13, 2015

beck24 commented Mar 13, 2015

Choose a reason for hiding this comment

mrclay commented Mar 14, 2015

beck24 commented Mar 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socialatm commented Mar 15, 2015

beck24 commented Mar 15, 2015

socialatm commented Mar 15, 2015

hypeJunction commented Mar 15, 2015

socialatm commented Mar 15, 2015

hypeJunction commented Mar 17, 2015

jeabakker commented Mar 17, 2015

mrclay commented Mar 17, 2015

socialatm commented Mar 17, 2015

mrclay commented Mar 17, 2015

ewinslow commented Mar 17, 2015

beck24 commented Mar 17, 2015

ewinslow commented Mar 17, 2015

hypeJunction commented Mar 17, 2015

socialatm commented Mar 17, 2015

beck24 commented Mar 17, 2015

socialatm commented Mar 17, 2015

ewinslow commented Mar 17, 2015

ewinslow commented Mar 17, 2015

socialatm commented Mar 17, 2015

beck24 commented Mar 17, 2015

beck24 commented Mar 18, 2015

mrclay commented Mar 18, 2015

socialatm commented Mar 18, 2015

socialatm commented Mar 19, 2015

mrclay commented Mar 19, 2015

mrclay commented Mar 19, 2015

beck24 commented Mar 20, 2015