Overhaul model visibility scoping #1342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Previously post visibility scoping required concrete knowledge of the parent discussion, ie. you needed a Discussion model on which you would call `postsVisibleTo($actor)`. This meant that to fetch posts from different discussions (eg. when listing user posts), it was a convoluted process, ultimately causing #1333. Now posts behave like any other model in terms of visibility scoping, and you simply call `whereVisibleTo($actor)` on a Post query. This scope will automatically apply a WHERE EXISTS clause that scopes the query to only include posts whose discussions are visible too. Thus, fetching posts from multiple discussions can now be done in a single query, simplifying things greatly and fixing #1333. - As such, the ScopePostVisibility event has been removed. Also, the rest of the "Scope" events have been consolidated into a single event, ScopeModelVisibility. This event is called whenever a user must have a certain $ability in order to see a set of discussions. Typically this ability is just "view". But in the case of discussions which have been marked as `is_private`, it is "viewPrivate". And in the case of discussions which have been hidden, it is "hide". etc. The relevant API on AbstractPolicy has been refined, now providing `find`, `findPrivate`, `findEmpty`, and `findWithPermission` methods. This could probably do with further refinement and we can re-address it once we get around to implementing more Extenders. - An additional change is that Discussion::comments() (the relation used to calculate the cached number of replies) now yields "comments that are not private", where before it meant "comments that are visible to Guests". This was flawed because eg. comments in non-public tags are technically not visible to Guests. Consequently, the Approval extension must adopt usage of `is_private`, so that posts which are not approved are not included in the replies count. Fundamentally, `is_private` now indicates that a discussion/ post should be hidden by default and should only be visible if it meets certain criteria. This is in comparison to non-is_private entities, which are visible by default and may be hidden if they don't meet certain criteria. Note that these changes have not been extensively tested, but I have been over the logic multiple times and it seems to check out.
|
I'm a bit packed with work right now. I might be able to take a look at the end of the week. If you want to push forward feel free to send in a PR before then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should fix #1333 and is required to properly implement visibility permissions for searching (see #1339).
Note that these changes have not been extensively tested, but I have been over the logic multiple times and it seems to check out. (famous last words)
See associated changes in:
flarum/approval@64d1ef0
flarum/tags@74c4430
@luceos These changes will break byobu. A lot. Once this is merged, if you'd like I can try and send a PR for byobu soon, since we will need it upgraded in order to deploy to discuss.flarum.org anyway.