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

Do not update permissions or attributes if other share fields gets updated #313

Merged
merged 3 commits into from
Jan 31, 2020

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Jan 18, 2020

Update share can update any field in share interface separately. This MR ensures that attributes get updated only if attribute or permission fields got updated.

  • wait until 10.4.0-rc2 gets released to test manually

@codecov
Copy link

codecov bot commented Jan 18, 2020

Codecov Report

Merging #313 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #313   +/-   ##
========================================
  Coverage      4.46%   4.46%           
  Complexity      306     306           
========================================
  Files            14      14           
  Lines          1188    1188           
========================================
  Hits             53      53           
  Misses         1135    1135

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffc162...ca61f2e. Read the comment docs.

@phil-davis phil-davis force-pushed the fix_36751 branch 2 times, most recently from 6887015 to a4887d3 Compare January 27, 2020 04:29
@phil-davis
Copy link
Contributor

With manual testing this looks to work nicely. Whenever "Secure view" is checked, all the other permissions are switched off (= read-only). Whenever some other permission is checked, secure view is switched off.

Rebased to trigger CI.

The workflow of the permissions checkboxes and expiration date is a bit different from what is in tests. Probably CI will fail - issue #316 I will look at adjusting the acceptance tests.

@phil-davis
Copy link
Contributor

As I see it now, when a user creates a share they can share with:

  1. ordinary sharing permissions and not secure-view - and then the sharee (if re-share permission was granted) can re-share just like normal. Or they can re-share with just secure-view - that is fine, secure-view is "less than read-only".

  2. secure-view - the sharee gets the share, can view the document(s) in the special app that renders the document with watermark... They cannot reshare it at all (not even reshare with secure-view permissions)

As far as I can see manually this is all working fine. I just need to adjust the acceptance test scenarios to cover the above kind of things.

@phil-davis
Copy link
Contributor

If we can get core PR owncloud/core#36847 merged today, then I will fix the acceptance tests here. (I don't want to bother fixing here until we at least think we have the stable solution finalized in core)

@phil-davis phil-davis self-assigned this Jan 31, 2020
@phil-davis
Copy link
Contributor

This turned out to be just a missing table headers row in the adjusted step of the failing scenario. I added a commit and it passes. I also made a core PR owncloud/core#36862 so that the test step gets our (recently) "standard" checks for this sort of problem.

@phil-davis phil-davis requested review from PVince81 and micbar January 31, 2020 11:48
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

WFM from a QA point-of-view.
I requested review from developers.
IMO we can merge this, then directly after 10.4 release we can also release this (and decide if we have to bump owncloud min-version="10.3" to 10.4 in appinfo/info.xml

@phil-davis phil-davis removed their assignment Jan 31, 2020
@phil-davis
Copy link
Contributor

@micbar I tagged @PVince81 for review. But request whoever you like.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

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.

4 participants