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

Fix #27851 - Copy option gets the correct Public Link #27863

Merged
merged 2 commits into from May 15, 2017

Conversation

Projects
None yet
4 participants
@arunjohnkuruvilla
Contributor

arunjohnkuruvilla commented May 11, 2017

Description

Multiple Public Links: "Copy" option gets always the same link. This is because the template which adds a new row when a public link is created refers to the link by {{cid}} instead of {{id}}.

Related Issue

#27851

Motivation and Context

Issue #27851 - Multiple Public Links: "Copy" option gets always the same link

How Has This Been Tested?

Manually.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant May 11, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented May 11, 2017

CLA assistant check
All committers have signed the CLA.

@DeepDiver1975 DeepDiver1975 requested a review from felixheidecke May 12, 2017

@DeepDiver1975 DeepDiver1975 added this to the 10.0.1 milestone May 12, 2017

@DeepDiver1975 DeepDiver1975 requested a review from PVince81 May 12, 2017

Show outdated Hide outdated core/js/sharedialoglinklistview.js
@@ -20,8 +20,8 @@
'<li class="link-entry" data-id="{{id}}">' +
'<span class="link-entry--icon icon-public-white"></span>' +
'<span class="link-entry--title">{{linkTitle}}</span>' +
'<div class="minify"><input id="linkText-{{cid}}" class="linkText" type="text" readonly="readonly" value="{{link}}" /></div>' +
'<div class="link-entry--icon-button clipboardButton" data-clipboard-target="#linkText-{{cid}}">' +
'<div class="minify"><input id="linkText-{{id}}" class="linkText" type="text" readonly="readonly" value="{{link}}" /></div>' +

This comment has been minimized.

@PVince81

PVince81 May 12, 2017

Member

I suggest to use both {{cid}}-{{id}}.

cid guarantees that the element id is unique on the whole page.

@PVince81

PVince81 May 12, 2017

Member

I suggest to use both {{cid}}-{{id}}.

cid guarantees that the element id is unique on the whole page.

This comment has been minimized.

@arunjohnkuruvilla

arunjohnkuruvilla May 13, 2017

Contributor

{{id}} is the id value for the particular share link returned from the backend. A share's id is kept unique by the backend. That should guarantee that the element id is unique for the whole application as well as the page.

@arunjohnkuruvilla

arunjohnkuruvilla May 13, 2017

Contributor

{{id}} is the id value for the particular share link returned from the backend. A share's id is kept unique by the backend. That should guarantee that the element id is unique for the whole application as well as the page.

This comment has been minimized.

@PVince81

PVince81 May 15, 2017

Member

What if there is another hidden sidebar that was rendered for another file list ? The sidebar might also contain the same elements with the same share ids but with a different view id.

This situation can usually happen if you navigate through the left sidebar elements "All files", "Shared with others" and open the sidebar for the same file/folder. When navigating away, the views aren't fully destroyed.

To keep it safe I'd prefer using {{cid}}-{{id}}. There were already bugs in the past where checkboxes/labels didn't work due to the condition above.

@PVince81

PVince81 May 15, 2017

Member

What if there is another hidden sidebar that was rendered for another file list ? The sidebar might also contain the same elements with the same share ids but with a different view id.

This situation can usually happen if you navigate through the left sidebar elements "All files", "Shared with others" and open the sidebar for the same file/folder. When navigating away, the views aren't fully destroyed.

To keep it safe I'd prefer using {{cid}}-{{id}}. There were already bugs in the past where checkboxes/labels didn't work due to the condition above.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 15, 2017

Member

Would be good to get this finished until tomorrow as we'll build the RC version then.

Let me know if you can't and I'll take over. Thanks.

Member

PVince81 commented May 15, 2017

Would be good to get this finished until tomorrow as we'll build the RC version then.

Let me know if you can't and I'll take over. Thanks.

@arunjohnkuruvilla

This comment has been minimized.

Show comment
Hide comment
@arunjohnkuruvilla

arunjohnkuruvilla May 15, 2017

Contributor

I have added the view identifier {{cid}} to the individual copy identifier. This should give a unique identifier every time.

Contributor

arunjohnkuruvilla commented May 15, 2017

I have added the view identifier {{cid}} to the individual copy identifier. This should give a unique identifier every time.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 15, 2017

Member

Awesome! Tested, works 👍

Member

PVince81 commented May 15, 2017

Awesome! Tested, works 👍

@PVince81 PVince81 merged commit 8f0d125 into owncloud:master May 15, 2017

1 of 5 checks passed

Scrutinizer Running
Details
continuous-integration/jenkins/pr-head This commit is being built
Details
continuous-integration/styleci/pr The StyleCI analysis is running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment