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

WIP: add support for public link roles assigned by apps #40289

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Aug 15, 2022

Description

Feature: Add support for public link roles assigned by apps

TODO

Related Issue

How Has This Been Tested?

  • manually in the developer instance
  • unit/acceptance tests

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Base code changes
  • Unit tests added
  • Acceptance tests added
  • Changelog item, see TEMPLATE

Example use-case: richdocuments preview

Screenshot 2022-08-15 at 21 52 53

Screenshot 2022-08-15 at 21 51 56

@mrow4a mrow4a self-assigned this Aug 15, 2022
@update-docs
Copy link

update-docs bot commented Aug 15, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

},
{
name: "allowPublicFileReadWrite",
permissions: OC.PERMISSION_READ | OC.PERMISSION_UPDATE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DeepDiver1975 public file read/write cannot have OC.PERMISSION_CREATE and OC.PERMISSION_DELETE , I fixed it here. Otherwise we could allow for delete of public link file from public link , and create obviously does not make sense.

@@ -22,31 +30,33 @@
'<label class="public-link-modal--label">{{linkNameLabel}}</label>' +
'<input class="public-link-modal--input" type="text" name="linkName" placeholder="{{namePlaceholder}}" value="{{name}}" maxlength="64" />' +
'</div>' +
'<div id="appManagedPublicLinkModelItems">' +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a place there apps can register new checkboxes

@@ -277,7 +277,7 @@ protected function formatShare(IShare $share, $received = false) {

$result['attributes'] = null;
if ($attributes = $share->getAttributes()) {
$result['attributes'] = \json_encode($attributes->toArray());
$result['attributes'] = $attributes->toArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This somehow sneaked in and is a bug, but would need full ci to run to confirm

@@ -882,6 +882,7 @@ public function updateShare($id) {
// legacy
$newPermissions !== (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE) &&
// correct
$newPermissions !== (Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE) &&
Copy link
Contributor Author

@mrow4a mrow4a Aug 15, 2022

Choose a reason for hiding this comment

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

@DeepDiver1975 public link file read/write does not work without this line. It was possible to create such share, but not update.

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 15, 2022

@phil-davis I changed quite some stuff in the HTML code so I expect a lot of JS and acceptance tests fails

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline webUIWebdavLockProt-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/36574/160

@phil-davis
Copy link
Contributor

@phil-davis I changed quite some stuff in the HTML code so I expect a lot of JS and acceptance tests fails

I can look at the acceptance test fails, and add a basic acceptance test for the new UI option. The test will verify that the public link does not have the ability to do an "ordinary" download or upload.

Should this public link feature be able to be selected for a folder? (and thus apply to all files/folders "under" that folder?)

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 17, 2022

@phil-davis @pmaier1 no it only works on single files (collabora formats more specifically)

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 18, 2022

@phil-davis btw, new option is only available with richdocuments app installed, and secure view enabled.

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 18, 2022

@phil-davis maybe in here make sense to just confirm no other feature is broken because of this one

cc @pmaier1

@phil-davis
Copy link
Contributor

@phil-davis maybe in here make sense to just confirm no other feature is broken because of this one

yes, for acceptance tests I will just make a scenario that checks that the new UI element exists and that it can be clicked to create a link...

@mrow4a are you going to make the unit tests (JS and PHP) pass?

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 19, 2022

@phil-davis this is the point, new Preview option will NOT show unless registered by apps. In this case richdocuments with Secure View emabled

@phil-davis
Copy link
Contributor

@phil-davis this is the point, new Preview option will NOT show unless registered by apps. In this case richdocuments with Secure View emabled

OK - now I understand why it will be hard to test just in core CI.

@NannaBarz
Copy link

@mrow4a what should we do here now in the Server Team Board?

@mrow4a
Copy link
Contributor Author

mrow4a commented Oct 10, 2022

@NannaBarz decide whether we want to have this feature or not.. ref. https://github.com/owncloud/enterprise/issues/5277

@pmaier1
Copy link
Contributor

pmaier1 commented Oct 10, 2022

@NannaBarz decide whether we want to have this feature or not

Yes, we want to have it for 10.12.

@mmattel
Copy link
Contributor

mmattel commented Jan 19, 2023

I am highly confident that this needs documentation when merged.
Pls file an issue in docs-server before merged.

@phil-davis
Copy link
Contributor

phil-davis commented Jan 26, 2023

@mrow4a there is mention above about "Yes, we want to have it for 10.12"

Is that still true?

https://github.com/owncloud/core/tree/e5277
There are conflicts reported, and it needs a rebase:
This branch is 3 commits ahead, 630 commits behind master.

@mrow4a
Copy link
Contributor Author

mrow4a commented Jan 26, 2023

@phil-davis on-hold -> https://github.com/owncloud/enterprise/issues/5277#issuecomment-1381704115

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.

None yet

6 participants