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.

Sign up for GitHub

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

Jump to bottom

[Tests-Only] Add acceptance tests to share top-level of read-only storage #37262

Merged
merged 1 commit into from
May 8, 2020
Merged

[Tests-Only] Add acceptance tests to share top-level of read-only storage #37262

merged 1 commit into from
May 8, 2020

Conversation

jasson99
Copy link
Contributor

Description

Add acceptance tests to share top-level of read-only storage

Related Issue

#36803

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jasson99 jasson99 self-assigned this Apr 15, 2020
@codecov
Copy link

codecov bot commented Apr 15, 2020 •
edited
Loading

Codecov Report

Merging #37262 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37262   +/-   ##
=========================================
  Coverage     64.55%   64.56%           
  Complexity    19218    19218           
=========================================
  Files          1268     1268           
  Lines         75104    75104           
  Branches       1331     1331           
=========================================
+ Hits          48486    48493    +7     
+ Misses        26226    26219    -7     
  Partials        392      392
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.72% <ø> (+0.01%) 19218.00 <ø> (ø)
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/lib/Expiration.php 98.27% <0.00%> (+1.72%) 29.00% <0.00%> (ø%)
apps/files_versions/lib/Expiration.php 98.63% <0.00%> (+8.21%) 34.00% <0.00%> (ø%)

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 113f231...57ba655. Read the comment docs.

@jasson99 jasson99 force-pushed the webUiShareTopLevelOfReadOnlyStorage branch from 5dc4594 to 408a149 Compare April 15, 2020 11:12
@jasson99 jasson99 requested a review from phil-davis April 16, 2020 06:01
@jasson99 jasson99 requested a review from phil-davis April 17, 2020 03:19
phil-davis
phil-davis reviewed Apr 17, 2020
View reviewed changes
tests/acceptance/features/cliLocalStorage/createLocalStorageReadOnlyAndShare.feature Outdated
Comment on lines 36 to 40
Scenario: applicable user is not able to share the top-level of read-only storage
When user "user0" shares file "/local_storage1/file-in-local-storage.txt" with user "user1" using the sharing API
Then the HTTP status code should be "200"
And the OCS status code should be "100"
And as "user1" file "file-in-local-storage.txt" should exist
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised that this passes. But maybe when the share is created without specifying any permissions, then the server automatically sets read-only permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you could add a scenario that explicitly tries to share the file with all permissions, and that should fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the share is created with all permissions when nothing is specified..That's what makes the difference in scenario:18 and scenario:24. And I also tried with all permissions declared explicitly and it passes. so I thought of not writing two different scenarios for the same thing, with default permissions, and with all permissions specified. What do you think?

@jasson99 jasson99 requested a review from phil-davis April 17, 2020 08:38
@jasson99 jasson99 requested a review from dpakach April 22, 2020 03:22
@phil-davis phil-davis force-pushed the webUiShareTopLevelOfReadOnlyStorage branch from 6c4f163 to 57ba655 Compare May 8, 2020 05:36
@phil-davis
Copy link
Contributor

I rebased and squashed the commits. This is ready-to-merge when CI passes.

@phil-davis phil-davis merged commit 2bb3236 into master May 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the webUiShareTopLevelOfReadOnlyStorage branch May 8, 2020 08:46
This was referenced May 9, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Talank Talank Talank left review comments

@phil-davis phil-davis phil-davis approved these changes

@haribhandari07 haribhandari07 Awaiting requested review from haribhandari07

@swoichha swoichha Awaiting requested review from swoichha

@dpakach dpakach Awaiting requested review from dpakach

Assignees

@jasson99 jasson99

Labels
4 - To release dev:acceptance-tests QA:team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasson99 @phil-davis @dpakach @Talank