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 reply on locked thread when there is a custom permissions #18

Closed
wants to merge 2 commits into from

Conversation

shivanshuit914
Copy link

Currently users can post reply on locked thread when there is a custom permission for specific tag discussions.

luceos
luceos previously approved these changes Nov 8, 2019
@dsevillamartin
Copy link
Member

Won't this code just affect the frontend? Ignoring the fact that this should not be needed because of the DiscussionPolicy, doesn't the serializer just affect the JSON attributes that are sent to the API, not when they are obtained directly from the model?

https://github.com/flarum/lock/blob/master/src/Access/DiscussionPolicy.php#L30-L35

@luceos
Copy link
Member

luceos commented Nov 11, 2019

@datitisev good point, I forgot about that. So what the PR needs to do is mutate the DiscussionPolicy instead of just the API attribute.

@luceos luceos self-requested a review November 11, 2019 12:58
@dsevillamartin
Copy link
Member

@luceos The DiscussionPolicy already contains the code necessary to check for if the user can lock, if that's what you mean - I think this is where we hit the issue with extension priority and all that, caused by flarum/tags.

@luceos luceos dismissed their stale review November 15, 2019 08:53

see comments by David

@clarkwinkelmann
Copy link
Member

Could we get a proper issue for this ? I'm unable to replicate, I'll need the exact steps.

If the issue does exist, then this PR doesn't solve anything as it's only affecting the frontend as discussed above. The fix will likely happen inside the discussion policy.

@shivanshuit914 or @luceos (if you replicated) can you please post the steps to reproduce ?

I suspect we'll have to push this to the next release cycle.

@franzliedke
Copy link
Contributor

Agreed, this needs to be solved at the policy level, assuming it is a reproducible problem.

Removing this from beta.11.

@askvortsov1
Copy link
Sponsor Member

This seems to be another instance of flarum/framework#1832, and will be fixed via flarum/framework#2056. I'm going to close this for now, thank you though!

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

Successfully merging this pull request may close these issues.

6 participants