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

Copy function for view only pages #1202

Merged
merged 3 commits into from Mar 9, 2019

Conversation

2 participants
@mark-james
Copy link
Contributor

mark-james commented Dec 31, 2018

Hi @ssddanbrown ,
I'd like to open a pull request to handle the ability to copy a read only page as discussed in #1199

This request is not yet complete.

Currently if a user has at least one of 'create page own' or 'create page all' then the option to copy the page is available. I deemed this as a prerequisite of any copy functionality.

There are (at least) two outstanding issues.
A user doesn't select a destination and pushes the copy button. And a variation when a user attempts to copy a page but does not have an existing destination available to copy it too.

How do you want to handle this?

What else do I need to consider?

mark-james added some commits Dec 30, 2018

Merge pull request #1 from BookStackApp/master
Update From Bookstack
@ssddanbrown

This comment has been minimized.

Copy link
Member

ssddanbrown commented Jan 1, 2019

Thanks for offering this pull request @mark-james. To properly cover the issue this is going to need some deeper changes.

We can't simply check the page-create-* or page-view role permissions, Since these changes have the possibility to show the actions for those that cannot actually use them. These permissions also don't take into account the entity-level permissions.

BookStack keeps a record of role-to-entity permissions in the joint_permissions table. This will need to be looked-up for each of the current user's roles.

@mark-james

This comment has been minimized.

Copy link
Contributor Author

mark-james commented Jan 2, 2019

Thanks for the feedback @ssddanbrown , I'll use the joint_permissions table as you suggest.

I'm a little fuzzy on why. Is it because just using the role permissions doesn't account for pages that have been overridden? Is this what you mean by entity level?

I'm assuming taking this approach also handles at least part of the issue I raised initially - "when a user attempts to copy a page but does not have an existing destination available to copy it too"? Eg. They can create pages but don't currently have a available book to do so.

Finally how do you want me to handle the scenario when a user doesn't select a destination to copy the page too. We could either -

  1. Disable the copy button until something is selected.
  2. Default to the first destination on the list
  3. Show a popup error message indicating the user doesn't have permission to copy the page to the existing book so they need to select a destination.
  4. Or is there another alternative?
@mark-james

This comment has been minimized.

Copy link
Contributor Author

mark-james commented Jan 2, 2019

Hi @ssddanbrown , I've updated to use the joint_permissions table. It's all done in one query so it should be performant. I've confirmed that it works with overridden permissions.

Have I understood the requirements correctly?

@ssddanbrown ssddanbrown added this to the v0.25.1 milestone Jan 3, 2019

@ssddanbrown ssddanbrown modified the milestones: v0.25.1, v0.25.2 Jan 20, 2019

@ssddanbrown ssddanbrown merged commit 19770d2 into BookStackApp:master Mar 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ssddanbrown

This comment has been minimized.

Copy link
Member

ssddanbrown commented Mar 9, 2019

Thanks for this @mark-james, Sorry for the delay in my response.

Your latest changes did exactly what I desired in my last comment so all way good-to-go.
I did abstract the implementation since we'll be doing similar checks for other permissions and I also added a test. My changes can be seen in 5c9b528.

The will be in the next patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.