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

show reshare public links to the original share owner #36865

Merged
merged 4 commits into from
Mar 4, 2020

Conversation

karakayasemi
Copy link
Contributor

@karakayasemi karakayasemi commented Feb 1, 2020

Description

With this PR, reshare public links will be visible for the original share-owner.
There will be no difference in the UI between owner-created public links and reshare public links. However, if we want to show reshares to the owner properly, we need a solution for all clients (desktop, mobile, web). The desktop client already shows this information, no need to hide them in here.

Related Issue

Motivation and Context

How Has This Been Tested?

  • create a reshare public link
  • it should be visible for original share owner

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

@karakayasemi karakayasemi added this to the development milestone Feb 1, 2020
@karakayasemi karakayasemi self-assigned this Feb 1, 2020
@update-docs
Copy link

update-docs bot commented Feb 1, 2020

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.

@jvillafanez
Copy link
Member

I'd wait for PM to take a decision here. Taking into account the comment, I guess the behaviour was intended and we should provide a way to distinguish both cases.

If everyone agree with the current change, I think it's better to leave a comment if the expected solution is around that piece of code.

@karakayasemi
Copy link
Contributor Author

@jvillafanez PM is okay with changes, we will show re-sharer properly in Phoenix. Also, @voroyam verified the fix.

Since current intended behavior identified as a bug, I do not see much benefit in adding a comment to the code.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

everyone agree, so let's move on.

@phil-davis
Copy link
Contributor

Even though there was a force-push 1 hour ago, this branch https://github.com/owncloud/core/tree/show-reshare-links still says that it has a .drone.yml

@karakayasemi It needs to be rebased right up-to-date with master - make sure to pull latest master locally first, then checkout this branch and rebase and force-push.

@karakayasemi
Copy link
Contributor Author

No idea how can I increase the coverage of deleted lines. @micbar, @phil-davis maybe merge this with repo owner permission?

@phil-davis
Copy link
Contributor

This works for me with manual testing. We should really make some automated test to demonstrate this.

In the screenshot, Anne has shared with Bob, and Bob has created a public link of the received share. Anne can see the public link - good. There is no way for Anne to see that it is a public link "created by Bob".

Maybe that is a meaningless thing to ask? Does "the system" have the public link stored somewhere under "Bob", or is it "really in the database" a public link of Anne's?

Screenshot_2020-02-13 Files - ownCloud

@phil-davis
Copy link
Contributor

No idea how can I increase the coverage of deleted lines. @micbar, @phil-davis maybe merge this with repo owner permission?

It is easy to override the codecov result - if other's are OK with that, @micbar ?

@karakayasemi
Copy link
Contributor Author

Maybe that is a meaningless thing to ask? Does "the system" have the public link stored somewhere under "Bob", or is it "really in the database" a public link of Anne's?

I did not fully understand the question but I can say, we store public links with original_share_owner and share_owner properties (they are available as uid_owner and uid_file_owner in this API response https://doc.owncloud.com/server/developer_manual/core/apis/ocs-share-api.html#get-all-shares).

However, properly showing this information in web UI needs design decisions and out of the scope of this bug-fix PR.

@phil-davis
Copy link
Contributor

@kiranparajuli589 maybe it will be easy to add a webUI acceptance test scenario to this PR?
We should be OK to just use API steps to setup:

  • user0 created "somefolder"
  • user0 shares to user1,
  • user1 shares to user2,
  • user1 also makes a public link share
  • login as user0
  • open the sharing panel of "somefolder"
  • check that the shares to user1 and user2 are seen
  • click the public links tab
  • check that the public link (created by user1) is seen (in the display of user0)

Please do it yourself, or assign someone.

@owncloud owncloud deleted a comment from codecov bot Feb 14, 2020
@kiranparajuli589 kiranparajuli589 self-assigned this Feb 14, 2020
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.

I realise this is WIP. These 2 things will need to be adjusted @kiranparajuli589

@owncloud owncloud deleted a comment from codecov bot Feb 14, 2020
@kiranparajuli589 kiranparajuli589 force-pushed the show-reshare-links branch 3 times, most recently from 5b1c2d7 to 65aa689 Compare February 14, 2020 15:54
@owncloud owncloud deleted a comment from codecov bot Feb 14, 2020
@owncloud owncloud deleted a comment from codecov bot Feb 14, 2020
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.

The folder/file changes suggested do not make a difference to the test run (the step will work either way.
But it is confusing to read has shared folder "/lorem.txt"

@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Feb 14, 2020

The folder/file changes suggested do not make a difference to the test run (the step will work either way.
But it is confusing to read has shared folder "/lorem.txt"

Thank you @phil-davis for your guidance :)

@owncloud owncloud deleted a comment from codecov bot Feb 14, 2020
@phil-davis
Copy link
Contributor

@kiranparajuli589 what is the status? Are you making progress here?

@kiranparajuli589
Copy link
Contributor

@kiranparajuli589 what is the status? Are you making progress here?

@phil-davis you need to review the test code (it has become quite stale)!
I've rebased the PR, CI is now runnig

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.

Also see the drone CI results - there are lots of failed pipelines.

And user "user0" has uploaded file with content "uploaded content" to "lorem.txt"
And user "user0" has shared file "lorem.txt" with user "user1"
And user "user1" has shared file "lorem.txt" with user "user2"
And user "user1" has created a public link share of file "lorem.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also have a scenario where both user0 and user1 have created a public link.
In that case, user0 should see 2 public link shares with name "Public link". You could make a Then step like:

Then :number public link shares with name :name should be visible

That is a case when the UI needs to display multiple public links that have the same "display name".

Copy link
Contributor

Choose a reason for hiding this comment

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

And then add a scenario where user0 deletes the public link(s), to make sure that user0 can delete the public link that was created by user1.

Copy link
Contributor

Choose a reason for hiding this comment

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

@phil-davis scenario is added.
please review!

@phil-davis
Copy link
Contributor

@karakayasemi if user0 "owner" and user1 "share receiver" of a resource both create a public link, then they both have the link name "Public link". user0 sees 2 "Public link" in the list. It works fine - good! But user0 has no way on the UI to understand what is the 2nd "Public link" in the list - e.g. they cannot see that the public link was created by user1.

Maybe that would be a separate feature, or not even a requirement at all?

@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36865   +/-   ##
=========================================
  Coverage     64.75%   64.76%           
  Complexity    19135    19135           
=========================================
  Files          1270     1270           
  Lines         74909    74907    -2     
  Branches       1329     1328    -1     
=========================================
  Hits          48511    48511           
+ Misses        26007    26006    -1     
+ Partials        391      390    -1     
Flag Coverage Δ Complexity Δ
#javascript 54.18% <ø> (+0.01%) 0.00 <ø> (ø)
#phpunit 65.93% <ø> (ø) 19135.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 90bb5e9...ac5b851. Read the comment docs.

@karakayasemi
Copy link
Contributor Author

@karakayasemi if user0 "owner" and user1 "share receiver" of a resource both create a public link, then they both have the link name "Public link". user0 sees 2 "Public link" in the list. It works fine - good! But user0 has no way on the UI to understand what is the 2nd "Public link" in the list - e.g. they cannot see that the public link was created by user1.

Maybe that would be a separate feature, or not even a requirement at all?

@phil-davis I also noticed this and asked it to PM in here https://github.com/owncloud/enterprise/issues/3757#issuecomment-578405186 . It considered not a requirement of this bugfix ticket, it will be implemented properly in Phoenix.

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.

Tests look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants