making posts and discussions private

[luceos]

Any discussion or post that has is_private to 1 is now hidden. Added phpdoc, casting and checked the gates.

Relates to FriendsOfFlarum/byobu#11

[tobyzerner]

Seems good, can you confirm that this allows you to make your extension behave the way you want it to?

[luceos]

Yes I can set the boolean to true and it won't show up discussions/posts. Other extensions are able to modify the property too, as planned.

[tobyzerner]

use false instead of 0 to be consistent with the PostPolicy one

[luceos]

Not sure where that oversight came from. Good eyes @tobscure :D

[tobyzerner]

can we please use the Migration::addColumns shortcut, as in https://github.com/flarum/core/blob/master/migrations/2015_10_24_194000_add_ip_address_to_posts.php

[luceos]

This new format is very unlike Laravel. What to provide as array values is pretty much not transparent @flarum/core ..

[tobyzerner]

@luceos we might also need to think about having an event to determine if a discussion/post is private or not. Imagine if there are two extensions that make things private, which are both enabled at once. Say I remove all members from a private discussion to make it public from the perspective of extension A... but extension B's "privatization" is still meant to apply. But extension A would've already flicked is_private to 0.

So instead of extensions directly manipulating the is_private field, they should just call $discussion->updatePrivate() or something, which would fire an event IsDiscussionPrivate, and then both extensions would listen and if either of them return true then the discussion is considered private.

Or maybe this is an edge case not worth worrying about? (at least for now)

[luceos]

@flarum/core how are we going to do a down migration now?

[tobyzerner]

@luceos the down migration is generated automatically by the helper, that's the whole point ;) Take a look at its implementation

[luceos]

Oops, I looked at it too hastily! Are these commits satisfactory?

[tobyzerner]

Should be. My apologies for not merging yet, I just want a chance to try it out on my local install first

[luceos]

No problem at all @tobscure I would most certainly also do that.

Requested changes have been implemented.

[franzliedke]

👍 from my side. Squash would be nice.

[tobyzerner]

@luceos So the reason I wanted to test this out before merging was to confirm my suspicion that it won't actually work, specifically when you try to reverse the condition in an extension like byobu.

This is indeed the case. If you look at it carefully, the DiscussionPolicy::find is adding WHERE conditions to restrict discussion visibility. By default, all discussions are visible; the conditions filter out discussions that the user shouldn't be able to see.

Therefore, once a WHERE condition is applied to that query, it is impossible to reverse it. The exception to this rule is when a discussion is hidden, in which case extensions have access to the ScopeHiddenDiscussionVisibility event to reverse the invisibility.

But in this case, no matter what conditions you add via a new policy/ScopeModelVisibility, the query ends up looking like this:

SELECT * 
FROM   `discussions` 
WHERE  (your conditions here)
       AND (other stuff pertaining to tags etc.)
       AND `discussions`.`is_private` = 0

ie. if is_private = 1, then that discussion is invisible and there's nothing that an extension like byobu can do about it.

Hence my trying to ask explicitly earlier: "can you confirm that this allows you to make your extension behave the way you want it to?" — I was hoping that you would have tested out a new version of byobu that tries (and fails) to work with this change.

Especially when tinkering with permissions and new extension APIs, it's never sufficient to look at the logic and assume it'll work. Please actually test it out in a real extension in the future!

---

As for a solution, I think we'll need to add a new ScopePrivateDiscussionVisibility event, in the same spirit as ScopeHiddenDiscussionVisibility. ie:

$query->where(function ($query) use ($actor) {
    $query->where('discussions.is_private', false);

    $this->events->fire(
        new ScopePrivateDiscussionVisibility($query, $actor)
    );
});

That way extensions can listen and add OR WHERE conditions to that subquery.

[luceos]

Yes makes sense, I should have tested that implementation as well. Let me take a shot at that implementation right now @tobscure

In hindsight I was assuming I would be mutating the is_private column on a discussion, not sure where that came from 😕

[luceos]

Based on the latest code in this PR I've made a refactor of the code in Byobu as preparation for b7. This logic makes the code of Byobu a lot easier. I had to invert some checks, modify the event listening. But all is looking well!

[luceos]

One consideration I'm not too sure about, do we want to mark posts in a private discussion private as well by default?

[tobyzerner]

@luceos Great work, thanks!

do we want to mark posts in a private discussion private as well by default?

Not necessary as a user can never see posts if they can't see the discussion that they're in.

[sijad]

as @tobscure mentioned earlier, is_private is very common, and it might get conflicted with multiple extension using it. isn't better to add type column or lets extension to define its required columns?

[tobyzerner]

What do you mean @sijad?

[sijad]

I'm not sure if it's good idea at all, but an extension can use discussions/posts implementations for other purposes like PM, users chat, etc. I thought adding a type to discussion can be useful for future.

[tobyzerner]

I think letting extensions add their own columns (eg. is_chat) will be ok

[dav-is]

The problem with adding is_chat and modifier columns like that is that if there are type specific columns added like chat_duration might be mysql type of int but would have to be nullable (say you required this to be a chat and didn’t want this to be nullable) which could potentially corrupt the mysql schema. Mysql would allow you to insert a chat but not require a chat_duration. I understand there’s some practical issues with my example, but just because I can’t come up with a perfect example doesn’t mean it doesn’t exist.

[tobyzerner]

@dav-is I don't see that as a big issue - just deal with it gracefully on the PHP side, or if you really want MySQL validation make a new table called "chats", store data in there, and link it back to the discussions table. Or do you have any other solutions?

[dav-is]

Why not encourage the use of a different table for every object. Why would we store chats in the posts table? And it’s not specifically about issues, but a clear organization as well. We can create something like Wordpress or smf where you just shove whatever into the database whererver the extension developer wants, or we could create a clear convention for devs to follow. Say the dev of a chat extension creates a faulty migration that drops the entire posts table instead of just the chats in the posts table.

[luceos]

I agree with @dav-is . Extensions should use their own tables and creating a convention to follow will create a better ecosystem.

is_private was meant to be applicable globally though, so I have no issue with keeping it like that.