Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add viewPrivate visibility scope #3596

Draft
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

imorland
Copy link
Member

@imorland imorland commented Aug 12, 2022

**Fixes #3594 **

Changes proposed in this pull request:
Allow discussions to be conditionally visible via viewPrivate scopers, when the user does not have the editPosts permission.

I feel that improving the tests for flarum/approval is also required, unfortunately I cannot run them locally atm, so perhaps someone would be kind enough to add?

Reviewers should focus on:
I've tested this change with 3rd party extensions, such as fof/byobu, which also makes use of viewPrivate, and there does not appear to be any side effects, but perhaps there's something I've not considered by opening this up a little?

Screenshot

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@imorland imorland self-assigned this Aug 12, 2022
@imorland imorland added this to the 1.5 milestone Aug 12, 2022
@imorland imorland marked this pull request as ready for review August 12, 2022 16:25
@imorland imorland requested review from luceos and SychO9 August 12, 2022 16:25
@SychO9
Copy link
Member

SychO9 commented Aug 17, 2022

Looked through the code a bit and from what I can understand, the idea of that block of code in the scoper is to hide discussions where the first post is private (waiting approval or hidden or anything else of the sort).

In which case I think it would be better to drop the editPosts scope completely and instead modify the logic of the first block of code in the scoper (view private logic).

However, whatever solution we try t go with, we need to setup the necessary related tests first.

@SychO9
Copy link
Member

SychO9 commented Aug 21, 2022

I set up the integration tests for the behavior we want to keep and the behavior we want to fix, so one of the tests currently naturally fails (even with the initial fix). This will make it easier to dig into the current logic and how we might want to tweak it.

@SychO9 SychO9 modified the milestones: 1.5, 1.6 Sep 6, 2022
@SychO9 SychO9 modified the milestones: 1.6, 1.7 Nov 15, 2022
@SychO9 SychO9 modified the milestones: 1.7, 1.8 Mar 6, 2023
@luceos luceos modified the milestones: 1.8, 1.x May 20, 2023
@SychO9 SychO9 removed this from the 1.x milestone May 22, 2023
@SychO9 SychO9 changed the base branch from main to 2.x May 27, 2023 17:43
@SychO9 SychO9 requested a review from a team as a code owner May 27, 2023 17:43
@askvortsov1
Copy link
Member

Was this intended to be marked as ready for review?

@SychO9
Copy link
Member

SychO9 commented May 28, 2023

@askvortsov1 nope! not sure how that happened 😅

@luceos
Copy link
Member

luceos commented Nov 13, 2024

With tests being done, is this something we can pull into 2.0 or 2.1 respectively @imorland @SychO9 ? It feels as this change isn't far from being ready for review?

@SychO9 SychO9 added this to the 2.1 milestone Nov 15, 2024
@SychO9
Copy link
Member

SychO9 commented Nov 15, 2024

This is low priority, can wait until 2.x minor releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Approving flagged posts / Edit posts permission
4 participants