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

Add permission to bypass throttling #1938

Merged
merged 1 commit into from
Feb 7, 2020

Conversation

dsevillamartin
Copy link
Member

@dsevillamartin dsevillamartin commented Nov 17, 2019

Fixes #1255.
Continues #1586.

  • Permission is called floodgate.postWithoutThrottle
    • Should it go under "moderation" instead? Seems to be more of a utility for those with power than normal members...
    • Should there be a different permission for starting discussions?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run php vendor/bin/phpunit).

Required changes:

  • Related core extension PRs: flarum/english (not done yet, waiting for confirmation)

@luceos
Copy link
Member

luceos commented Nov 18, 2019

  • Yes under moderation
  • No not for discussion. If memory serves me right, the floodgate is only active when posting.

@dsevillamartin
Copy link
Member Author

@luceos Ok, I'll move it to moderation.

If the discussion posting doesn't have a floodgate, why does the controller assert that it shouldn't throttle?

@luceos luceos added this to the 0.1.0-beta.11 milestone Nov 20, 2019
@luceos
Copy link
Member

luceos commented Nov 20, 2019

Feel free to merge once the pr is merged to English.

@dsevillamartin
Copy link
Member Author

@luceos 👍 What should the wording be? "Bypass posting floodgate"/"Bypass post throttling"...?

@jordanjay29
Copy link
Contributor

@luceos 👍 What should the wording be? "Bypass posting floodgate"/"Bypass post throttling"...?

How about "Reply multiple times without waiting" in keeping with our other Permission names?

@franzliedke franzliedke removed this from the 0.1.0-beta.11 milestone Nov 20, 2019
@franzliedke
Copy link
Contributor

Hmm, what about the permission name? That floodgate. prefix is new, not sure what the convention is there. But the discussion.replyWithoutApproval seems to be the closest existing thing, so maybe we should scope this one under discussion, too? 🤔

@tobyzerner If you happen to read this... can you explain the idea behind the naming scheme? Why are some of the permission names scoped, and others aren't?

@tobyzerner
Copy link
Contributor

They are scoped so that specific policies can check them. eg. DiscussionPolicy's can method.

@luceos
Copy link
Member

luceos commented Nov 21, 2019

Also only those permissions prefixed with discussion can be configured per tag.

@Ralkage
Copy link
Contributor

Ralkage commented Dec 13, 2019

I actually pondered on the idea of changing the default hard-coded time limit for the floodgate to something configurable as a group permission to either set it to 0 (bypass floodgate) or a set value (i.e. 30 seconds, 2 minutes, etc) but that may be more work to do I assume.

I'm referencing https://github.com/flarum/core/blob/d492579638fb52dafbfe65f1f36a95eb6047f7f3/src/Post/Floodgate.php#L45-L52 this method of course.

I like the approach to provide as much flexibility with permissions as possible as it can also be used as an award or restricted group permission respectively.

@dsevillamartin
Copy link
Member Author

@Ralkage There's probably an issue for that already, and it's outside the scope of this PR.

@franzliedke franzliedke force-pushed the ds/1255-throttling-bypass-permission branch 2 times, most recently from af8775d to b5c1e78 Compare February 7, 2020 14:27
@franzliedke
Copy link
Contributor

I removed the prefix in the permission name, and encapsulated the permission check in the Floodgate class itself.

Copy link
Contributor

@franzliedke franzliedke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy now! 😀

@franzliedke franzliedke force-pushed the ds/1255-throttling-bypass-permission branch from b5c1e78 to 711e775 Compare February 7, 2020 14:30
@franzliedke franzliedke merged commit b91e903 into master Feb 7, 2020
@franzliedke franzliedke deleted the ds/1255-throttling-bypass-permission branch February 7, 2020 14:34
franzliedke added a commit to flarum/lang-english that referenced this pull request Feb 7, 2020
askvortsov1 pushed a commit to flarum/lang-english that referenced this pull request Mar 11, 2022
askvortsov1 pushed a commit to flarum/lang-english that referenced this pull request May 10, 2022
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.

Throttling is bothersome to some permission groups
6 participants