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

Allow multiple link shares #27337

Merged
merged 41 commits into from Mar 23, 2017

Conversation

Projects
None yet
4 participants
@PVince81
Member

PVince81 commented Mar 8, 2017

Description

Removes the artifical restriction in the code that limited link shares to a single entry.

Please note that the limit was put in place because the UI wasn't ready for multiple link shares.
Before merging this, make sure the UI is adjusted first.

Related Issue

Motivation and Context

We want multiple link shares!

How Has This Been Tested?

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.

@fheidecke here you go, please use this branch when testing the frontend changes.
Multiple link shares can simply be created by using POST when creating the share and PUT when changing it. Each will get a different id. It's similar to regular user shares.

@PVince81 PVince81 added this to the 10.0 milestone Mar 8, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 8, 2017

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mjobst-necls and @DeepDiver1975 to be potential reviewers.

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mjobst-necls and @DeepDiver1975 to be potential reviewers.

@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Mar 10, 2017

Contributor

Does this support all features shown in this clickdummy?
https://felixheidecke.github.io/owc_clickdummys/pages/sharing.html

Contributor

felixheidecke commented Mar 10, 2017

Does this support all features shown in this clickdummy?
https://felixheidecke.github.io/owc_clickdummys/pages/sharing.html

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 14, 2017

Member

Does this support all features shown in this clickdummy?

very likely

Member

PVince81 commented Mar 14, 2017

Does this support all features shown in this clickdummy?

very likely

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 14, 2017

Member

Tasks:

  • create SharesCollection class for shares and use it only for link shares for now

  • create ShareModel class for shares but only use for link shares for now

    • fetch must get it
    • save must create or update it
  • make ShareItemModel's getLinkShare method return a collection instead of an array => getLinkShareCollection()

  • create a ShareLinkCollectionView representing the link shares list

    • based / wired with a SharesCollection (any event rerenders the whole list)
    • put it in a separate sub-tab "Public links" as per click-dummy
    • action for opening popup
      • set a newly created ShareModel
      • add it to collection if popup was closed with OK
    • action for deletion (collection.get(shareId).destroy())
    • action for edition: get the ShareModel from the collection and pass it to the popup
  • repurpose ShareDialogLinkShareView for the popup

    • remove all saving logic and move it to the model if needed (we only save on OK, not for every field change)
    • triggers events for OK and Cancel/close
    • based on a ShareModel
    • merge expiration subview into this one => did not merge it but used it
    • put it into a popup dialog
    • clicking OK calls shareModel.save() and closes
Member

PVince81 commented Mar 14, 2017

Tasks:

  • create SharesCollection class for shares and use it only for link shares for now

  • create ShareModel class for shares but only use for link shares for now

    • fetch must get it
    • save must create or update it
  • make ShareItemModel's getLinkShare method return a collection instead of an array => getLinkShareCollection()

  • create a ShareLinkCollectionView representing the link shares list

    • based / wired with a SharesCollection (any event rerenders the whole list)
    • put it in a separate sub-tab "Public links" as per click-dummy
    • action for opening popup
      • set a newly created ShareModel
      • add it to collection if popup was closed with OK
    • action for deletion (collection.get(shareId).destroy())
    • action for edition: get the ShareModel from the collection and pass it to the popup
  • repurpose ShareDialogLinkShareView for the popup

    • remove all saving logic and move it to the model if needed (we only save on OK, not for every field change)
    • triggers events for OK and Cancel/close
    • based on a ShareModel
    • merge expiration subview into this one => did not merge it but used it
    • put it into a popup dialog
    • clicking OK calls shareModel.save() and closes
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: make sure that when resharing is not allowed the UI appear properly accordingly
Member

PVince81 commented Mar 15, 2017

  • TODO: make sure that when resharing is not allowed the UI appear properly accordingly
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: the share link social view must only appear / be created when clicking the share/anchor icon (only a single view might be needed and append it within the row)
Member

PVince81 commented Mar 15, 2017

  • TODO: the share link social view must only appear / be created when clicking the share/anchor icon (only a single view might be needed and append it within the row)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: double check expire data inconsistency
  • TODO: test server side errors handling in link dialog (ex: password policy)
Member

PVince81 commented Mar 15, 2017

  • TODO: double check expire data inconsistency
  • TODO: test server side errors handling in link dialog (ex: password policy)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member

@felixheidecke please have a look at my changes, let me know if something is unclear.

We still need the subtab for "Public links".

Regarding the merging of the expiration view I'd recommend against it to save some time. If needed I can simply rewire it for now. Let me know what you think.

Member

PVince81 commented Mar 15, 2017

@felixheidecke please have a look at my changes, let me know if something is unclear.

We still need the subtab for "Public links".

Regarding the merging of the expiration view I'd recommend against it to save some time. If needed I can simply rewire it for now. Let me know what you think.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: email field

Should the email field rather be part of the anchor feature ? (share with social, share with email, copy to clipboard). It feels weird to have an email field in the popup and need to click "Edit" to access it.

Member

PVince81 commented Mar 15, 2017

  • TODO: email field

Should the email field rather be part of the anchor feature ? (share with social, share with email, copy to clipboard). It feels weird to have an email field in the popup and need to click "Edit" to access it.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: adjust unit tests (once the structure is set in stone)
Member

PVince81 commented Mar 15, 2017

  • TODO: adjust unit tests (once the structure is set in stone)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • only show email field in popup on create

  • test if email field actually works

  • investigate how to pass / add a new "label" field for link naming

Member

PVince81 commented Mar 15, 2017

  • only show email field in popup on create

  • test if email field actually works

  • investigate how to pass / add a new "label" field for link naming

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TEST: reshares and reshare info display
Member

PVince81 commented Mar 15, 2017

  • TEST: reshares and reshare info display
@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Mar 15, 2017

Contributor

It feels weird to have an email field in the pop-up and need to click "Edit" to access it.

As we discussed just now, we need this in the modal to be able to access the password for further usage with E-Mail and Text-Messages (SMS)

Contributor

felixheidecke commented Mar 15, 2017

It feels weird to have an email field in the pop-up and need to click "Edit" to access it.

As we discussed just now, we need this in the modal to be able to access the password for further usage with E-Mail and Text-Messages (SMS)

@PVince81 PVince81 referenced this pull request Mar 15, 2017

Merged

Move oc_share to migration #27394

4 of 9 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • REQUIRES: PR to have oc_share be built within a migration: #27394

Then once we have this we can use another migration to add the new column.

Member

PVince81 commented Mar 15, 2017

  • REQUIRES: PR to have oc_share be built within a migration: #27394

Then once we have this we can use another migration to add the new column.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member

I've already added the migration commit + the new name column + passing it through the APIs on this PR.
There's now a name field now in the popup which value is saved/edited properly.

Now if we want the name to be prepopulated to be the token, then it means we need to pre-create the share before opening the popup for editing. If we really want that, then once the share model reaches the popup it isn't a new share any more. So we'll need to pass a flag to the dialog as it cannot rely on isNew() any more.

@felixheidecke can you confirm that this is the flow we want ?

Member

PVince81 commented Mar 15, 2017

I've already added the migration commit + the new name column + passing it through the APIs on this PR.
There's now a name field now in the popup which value is saved/edited properly.

Now if we want the name to be prepopulated to be the token, then it means we need to pre-create the share before opening the popup for editing. If we really want that, then once the share model reaches the popup it isn't a new share any more. So we'll need to pass a flag to the dialog as it cannot rely on isNew() any more.

@felixheidecke can you confirm that this is the flow we want ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • creation date in tooltip ? (it's the "stime" attribute)
Member

PVince81 commented Mar 15, 2017

  • creation date in tooltip ? (it's the "stime" attribute)
@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Mar 15, 2017

Contributor

@PVince81 My suggestion is the following (in code terms): echo (name) ? name : token
There is no need to populate the name field if the name input is null. Agree?

Contributor

felixheidecke commented Mar 15, 2017

@PVince81 My suggestion is the following (in code terms): echo (name) ? name : token
There is no need to populate the name field if the name input is null. Agree?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member

My suggestion is the following (in code terms): echo (name) ? name : token
There is no need to populate the name field if the name is null. Agree?

Yes, agree.

Member

PVince81 commented Mar 15, 2017

My suggestion is the following (in code terms): echo (name) ? name : token
There is no need to populate the name field if the name is null. Agree?

Yes, agree.

@felixheidecke

This comment has been minimized.

Show comment
Hide comment
@felixheidecke

felixheidecke Mar 15, 2017

Contributor

creation date in tooltip?

Depends on where the tooltip lives. Go ahead and implement it. I can take care of it whenever I bügel glatt the UI.

Contributor

felixheidecke commented Mar 15, 2017

creation date in tooltip?

Depends on where the tooltip lives. Go ahead and implement it. I can take care of it whenever I bügel glatt the UI.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member

I'll let you do the share time stuff.

In the last push a7db599:

  • added mail view to the popup (only for new shares)
  • email is sent when clicking ok, right after share creation
  • removed the link share text field and replaced it with the name (with the logic name || token)

@felixheidecke I think you can take over from here. Let me know when the structures are stable enough so I can update most unit tests.

Member

PVince81 commented Mar 15, 2017

I'll let you do the share time stuff.

In the last push a7db599:

  • added mail view to the popup (only for new shares)
  • email is sent when clicking ok, right after share creation
  • removed the link share text field and replaced it with the name (with the logic name || token)

@felixheidecke I think you can take over from here. Let me know when the structures are stable enough so I can update most unit tests.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member

I've just added a few more tweaks:

  • link view is now rendered lazily
  • link view and subtabs properly disappear when link sharing is globally forbidden
  • properly display "resharing is not allowed" on the main view
  • prevent sidebar tab switching to affect subtabs (it was a bug in the sidebar code)
Member

PVince81 commented Mar 15, 2017

I've just added a few more tweaks:

  • link view is now rendered lazily
  • link view and subtabs properly disappear when link sharing is globally forbidden
  • properly display "resharing is not allowed" on the main view
  • prevent sidebar tab switching to affect subtabs (it was a bug in the sidebar code)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 15, 2017

Member
  • TODO: server-side unit tests for the newly added "name" column...
Member

PVince81 commented Mar 15, 2017

  • TODO: server-side unit tests for the newly added "name" column...
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 16, 2017

Member
  • TODO: reimplement password enforcement logic (visual clue about required field + validation on save)
Member

PVince81 commented Mar 16, 2017

  • TODO: reimplement password enforcement logic (visual clue about required field + validation on save)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 16, 2017

Member
  • BUG: clipboard button doesn't seem to work for hidden fields. Maybe need to hide it offscreen instead
Member

PVince81 commented Mar 16, 2017

  • BUG: clipboard button doesn't seem to work for hidden fields. Maybe need to hide it offscreen instead
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 16, 2017

Member
  • little clear button to clear fields, unless the field value is required
Member

PVince81 commented Mar 16, 2017

  • little clear button to clear fields, unless the field value is required
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 20, 2017

Member

We can't merge this yet as there is no proper way for copying the link.
Minimum would be to make the clipboard thing work to avoid losing this in a major way.

  • BUG: no way to copy link

In general I'd also like to have the icon order sorted out already before merging, if possible. Trash is still too close to the share button.

Member

PVince81 commented Mar 20, 2017

We can't merge this yet as there is no proper way for copying the link.
Minimum would be to make the clipboard thing work to avoid losing this in a major way.

  • BUG: no way to copy link

In general I'd also like to have the icon order sorted out already before merging, if possible. Trash is still too close to the share button.

Show outdated Hide outdated core/js/sharedialoglinklistview.js
t('core', 'Delete {link}', { link: linkTitle }),
t('core', 'Remove link'),
function(cb) {
if (cb)

This comment has been minimized.

@PVince81

PVince81 Mar 20, 2017

Member

please always add the brackets for readability

and also a // TODO: handle error

@PVince81

PVince81 Mar 20, 2017

Member

please always add the brackets for readability

and also a // TODO: handle error

Show outdated Hide outdated apps/files_sharing/css/sharetabview.css
height: 32px;
width: 32px;
background-color: #b4b4b4;
background-image: url('/core/img/actions/public-white.svg');

This comment has been minimized.

@PVince81

PVince81 Mar 20, 2017

Member

please revert back and leave my change, this breaks on every setup where ownCloud is not deployed under "/core"

why not use the "icon-public-white" class like I did ? that one has the correct path.

Also note that we must use the icon-* classes because some themes might override these.

And we should probable not use ":before" here but a real element because ":before" cannot be overridden by themes.

@PVince81

PVince81 Mar 20, 2017

Member

please revert back and leave my change, this breaks on every setup where ownCloud is not deployed under "/core"

why not use the "icon-public-white" class like I did ? that one has the correct path.

Also note that we must use the icon-* classes because some themes might override these.

And we should probable not use ":before" here but a real element because ":before" cannot be overridden by themes.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 20, 2017

Member
  • add hook inside the popup to allow apps to extend the dialog's behavior for example with new fields
Member

PVince81 commented Mar 20, 2017

  • add hook inside the popup to allow apps to extend the dialog's behavior for example with new fields
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 20, 2017

Member

I've added back the anchor icon which disappeared...

Also added extension point in popup dialog.

Fixed ShareModel.getLink() but unit test will fail... I originally changed that method to make the test pass but now it seems it's better to fix the test.

  • Fix ShareModel.getLink() unit test
Member

PVince81 commented Mar 20, 2017

I've added back the anchor icon which disappeared...

Also added extension point in popup dialog.

Fixed ShareModel.getLink() but unit test will fail... I originally changed that method to make the test pass but now it seems it's better to fix the test.

  • Fix ShareModel.getLink() unit test
Show outdated Hide outdated apps/files_sharing/lib/API/Share20OCS.php
@@ -329,16 +332,6 @@ public function createShare() {
return new \OC\OCS\Result(null, 404, $this->l->t('Public link sharing is disabled by the administrator'));
}
/*
* For now we only allow 1 link share.
* Return the existing link share if this is a duplicate

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

Have duplicated links been taken into account?

@jvillafanez

jvillafanez Mar 21, 2017

Member

Have duplicated links been taken into account?

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

Hmm good question... I think that what Roeland meant back then was simply "multiple links".
I don't think it's possible to have duplicate links which would mean having duplicate tokens.

@PVince81

PVince81 Mar 21, 2017

Member

Hmm good question... I think that what Roeland meant back then was simply "multiple links".
I don't think it's possible to have duplicate links which would mean having duplicate tokens.

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

I expect the DB would take care of this, but since we suppose to support several share providers I don't know what component should be responsible of the deduplication or prevent the duplication.

@jvillafanez

jvillafanez Mar 22, 2017

Member

I expect the DB would take care of this, but since we suppose to support several share providers I don't know what component should be responsible of the deduplication or prevent the duplication.

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

Ok, I'll simply write an additional test here #27448 (comment)

If it fails then we might need to make the index of the "token" column unique.

@PVince81

PVince81 Mar 22, 2017

Member

Ok, I'll simply write an additional test here #27448 (comment)

If it fails then we might need to make the index of the "token" column unique.

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

I did a test and apparently the token column has no unique index...

Also I think the unique token problem is not relevant for multiple link shares because the token is always generated by the server and not settable by the user.

However if someone uses the PHP API directly they can do that, but this is again not relevant to this PR here.

Raised #27458 to investigate separately.

@PVince81

PVince81 Mar 22, 2017

Member

I did a test and apparently the token column has no unique index...

Also I think the unique token problem is not relevant for multiple link shares because the token is always generated by the server and not settable by the user.

However if someone uses the PHP API directly they can do that, but this is again not relevant to this PR here.

Raised #27458 to investigate separately.

@@ -70,7 +70,6 @@
$countOfDataLocation = 0;
$value = $config->getAppValue('core', 'shareapi_enable_link_password_by_default', 'no');
$enableLinkPasswordByDefault = ($value === 'yes') ? true : false;

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

Is this on purpose?

@jvillafanez

jvillafanez Mar 21, 2017

Member

Is this on purpose?

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

Ah, yes...

Remember how the link creation used to work before this PR: it was sometimes like a step by step process especially when enforcement is set. Some contributor added this option to make the password field appear first to suggest more strongly to a user to set a password, but the user still had the option to leave it empty. (so it's less strong than "enforce password").

Since we modified the flow by having a dialog, this option becomes obsolete because there is no more step by step process.

@PVince81

PVince81 Mar 21, 2017

Member

Ah, yes...

Remember how the link creation used to work before this PR: it was sometimes like a step by step process especially when enforcement is set. Some contributor added this option to make the password field appear first to suggest more strongly to a user to set a password, but the user still had the option to leave it empty. (so it's less strong than "enforce password").

Since we modified the flow by having a dialog, this option becomes obsolete because there is no more step by step process.

This comment has been minimized.

Show outdated Hide outdated lib/private/Share20/Share.php
/**
* @inheritdoc
*/
public function setName($name) {

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

taking into account the limitation in the DB I'd rather check the length of the string and do something if it's over the limit.

@jvillafanez

jvillafanez Mar 21, 2017

Member

taking into account the limitation in the DB I'd rather check the length of the string and do something if it's over the limit.

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

This should rather be done in the DefaultShareProvider because that's the one close to the DB.
From what I see the current DefaultShareProvider's DB call would simply throw if the field is too big, which I think is fine for now... (ideally such exceptions should be encapsulated in a higher level exception type)

Here in this class we're on the abstract level and some providers might allow for bigger fields.

@PVince81

PVince81 Mar 21, 2017

Member

This should rather be done in the DefaultShareProvider because that's the one close to the DB.
From what I see the current DefaultShareProvider's DB call would simply throw if the field is too big, which I think is fine for now... (ideally such exceptions should be encapsulated in a higher level exception type)

Here in this class we're on the abstract level and some providers might allow for bigger fields.

This comment has been minimized.

@jvillafanez

jvillafanez Mar 22, 2017

Member

Yes, it makes more sense to check it in the DefaultShareProvider.
What worries me is that the exception isn't documented. Right now, if I use this function and try to set a big name I expect it works, not throwing an exception. There is nothing telling me "this could fail under these conditions"

@jvillafanez

jvillafanez Mar 22, 2017

Member

Yes, it makes more sense to check it in the DefaultShareProvider.
What worries me is that the exception isn't documented. Right now, if I use this function and try to set a big name I expect it works, not throwing an exception. There is nothing telling me "this could fail under these conditions"

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

ok, will take care here #27448 (comment) once I get Oracle stuff sorted out

@PVince81

PVince81 Mar 22, 2017

Member

ok, will take care here #27448 (comment) once I get Oracle stuff sorted out

Show outdated Hide outdated lib/public/Share/IShare.php
@@ -323,4 +323,20 @@ public function setMailSend($mailSend);
* @since 9.0.0
*/
public function getMailSend();
/**
* Set an arbitrary name for this share

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

To be decided how we're going to handle any limitation here. If we state here that the length should be 64 bytes at most, then it's expected both the implementations and anything using them will respect that limitation.
With the current documentation, the implementation have to take care of name over 64 bytes somehow, otherwise it's a bug in the implementation.

@jvillafanez

jvillafanez Mar 21, 2017

Member

To be decided how we're going to handle any limitation here. If we state here that the length should be 64 bytes at most, then it's expected both the implementations and anything using them will respect that limitation.
With the current documentation, the implementation have to take care of name over 64 bytes somehow, otherwise it's a bug in the implementation.

Show outdated Hide outdated lib/public/Share/IShare.php
public function setName($name);
/**
* Get arbitrary name for this share

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

This could lead to confusion. An arbitrary name could be any random name, not just the one set previously by setName, which is what anyone would expect.

@jvillafanez

jvillafanez Mar 21, 2017

Member

This could lead to confusion. An arbitrary name could be any random name, not just the one set previously by setName, which is what anyone would expect.

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

Indeed. I actually meant "user-defined" name. I'll adjust.

@PVince81

PVince81 Mar 21, 2017

Member

Indeed. I actually meant "user-defined" name. I'll adjust.

Show outdated Hide outdated tests/lib/Share20/DefaultShareProviderTest.php
@@ -1904,6 +1911,8 @@ public function testUpdateLink() {
$share->setShareOwner('user5');
$share->setNode($file2);
$share->setPermissions(1);
$share->setToken('anothertoken');
$share->setName('another_name');

This comment has been minimized.

@jvillafanez

jvillafanez Mar 21, 2017

Member

it should be possible to set null as name, or not to set a name, is this case taken into account?

@jvillafanez

jvillafanez Mar 21, 2017

Member

it should be possible to set null as name, or not to set a name, is this case taken into account?

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

null as name is possible

@PVince81

PVince81 Mar 22, 2017

Member

null as name is possible

$target.addClass('error');
}
});
var expiration = moment($target.val(), 'DD-MM-YYYY').format('YYYY-MM-DD');

This comment has been minimized.

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

date format should be language-specific. (even tho it wasn't before either ;-))

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

date format should be language-specific. (even tho it wasn't before either ;-))

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

Not now... this is likely to become a headache -> separate ticket.

We already tried to change this format in the past and it exploded in many places.

@PVince81

PVince81 Mar 21, 2017

Member

Not now... this is likely to become a headache -> separate ticket.

We already tried to change this format in the past and it exploded in many places.

var expiration;
if (isExpirationSet) {
expiration = moment(this.model.get('linkShare').expiration, 'YYYY-MM-DD').format('DD-MM-YYYY');
expiration = moment(this.model.get('expireDate'), 'YYYY-MM-DD').format('DD-MM-YYYY');

This comment has been minimized.

@@ -182,13 +146,30 @@
maxDate: maxDate
});
this.$el.find('.datepicker').datepicker({dateFormat : 'dd-mm-yy'});
this.$('.datepicker').datepicker({dateFormat : 'dd-mm-yy'});
_template: undefined,
events: {
'click .addLink': 'onAddButtonClick',

This comment has been minimized.

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

Are those classes scoped btw?

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

Are those classes scoped btw?

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

Yes, Backbone binds the specified handlers to the class so no need for extra binds.

@PVince81

PVince81 Mar 21, 2017

Member

Yes, Backbone binds the specified handlers to the class so no need for extra binds.

Show outdated Hide outdated core/js/sharedialoglinkshareview.js
if(event.keyCode == 13) {
this.onPasswordEntered();
if (this.configModel.get('enforcePasswordForPublicLink')
&& !password.trim()

This comment has been minimized.

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

Password isn't trim-ched above (https://github.com/owncloud/core/pull/27337/files#diff-da7df34db49581af77bd59e4f269dc36R97) but here it is. Only spaces and tabs could be valid password?!?!

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

Password isn't trim-ched above (https://github.com/owncloud/core/pull/27337/files#diff-da7df34db49581af77bd59e4f269dc36R97) but here it is. Only spaces and tabs could be valid password?!?!

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

I'll remove the trim bit.

@PVince81

PVince81 Mar 21, 2017

Member

I'll remove the trim bit.

}));
this.$('.datepicker').datepicker({dateFormat : 'dd-mm-yy'});
Show outdated Hide outdated core/js/sharedialoglinksocialview.js
' data-url="mailto:?subject=&body={{reference}}"></button>' +
' {{/if}}' +
'{{/if}}'
' data-url="mailto:?subject=&body={{reference}}"></button>'

This comment has been minimized.

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

subject isn't needed if left blank. https://tools.ietf.org/html/rfc2368

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

subject isn't needed if left blank. https://tools.ietf.org/html/rfc2368

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

copy-pasted from old code, remove ?

@PVince81

PVince81 Mar 21, 2017

Member

copy-pasted from old code, remove ?

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

will remove it

@PVince81

PVince81 Mar 21, 2017

Member

will remove it

*
* @param {string} recipientEmail recipient email address
*/
_sendEmailPrivateLink: function(recipientEmail) {

This comment has been minimized.

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

What does 'private' imply here?

@felixheidecke

felixheidecke Mar 21, 2017

Contributor

What does 'private' imply here?

This comment has been minimized.

@PVince81

PVince81 Mar 21, 2017

Member

No idea... I just copied the old method name.

I think it's still a public link but the email might be what is private...

@PVince81

PVince81 Mar 21, 2017

Member

No idea... I just copied the old method name.

I think it's still a public link but the email might be what is private...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

@jvillafanez adjustments here 106ff5c

Member

PVince81 commented Mar 21, 2017

@jvillafanez adjustments here 106ff5c

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member
  • TODO: raise ticket to consolidate and l10nify the date picker date

@felixheidecke adjustments here: 3117e47

Member

PVince81 commented Mar 21, 2017

  • TODO: raise ticket to consolidate and l10nify the date picker date

@felixheidecke adjustments here: 3117e47

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

Uh oh, some test failures:

13:43:54 1) Test\Share20\DefaultShareProviderTest::testUpdateLink
13:43:54 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'INSERT INTO `oc_share` (`share_type`, `uid_initiator`, `uid_owner`, `item_type`, `file_source`, `file_target`, `permissions`, `token`, `parent`) VALUES('3', 'user1', 'user2', 'file', '42', 'target', '31', 'tehtoken', 'some_name')':
13:43:54 
13:43:54 SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'some_name' for column 'parent' at row 

Seems something is shifted... But tests passed locally for me.

I'll have a look.

Member

PVince81 commented Mar 21, 2017

Uh oh, some test failures:

13:43:54 1) Test\Share20\DefaultShareProviderTest::testUpdateLink
13:43:54 Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'INSERT INTO `oc_share` (`share_type`, `uid_initiator`, `uid_owner`, `item_type`, `file_source`, `file_target`, `permissions`, `token`, `parent`) VALUES('3', 'user1', 'user2', 'file', '42', 'target', '31', 'tehtoken', 'some_name')':
13:43:54 
13:43:54 SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'some_name' for column 'parent' at row 

Seems something is shifted... But tests passed locally for me.

I'll have a look.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 21, 2017

Member

Unit tests fixed 80ce581

Member

PVince81 commented Mar 21, 2017

Unit tests fixed 80ce581

PVince81 and others added some commits Mar 16, 2017

First rough styling of multiple link shares,
add new color versions of some icons
Only send required attrs for share links
Also remove the password attribute when one is already set and the user
did not want to change it
Tweaks for link shares
Add back anchor icon in list.
Fix generated link.
Added extension point in dialog.
Make OC.webroot properly stubbable
Make other OC.* API calls use OC.getRootPath() instead of directly using
OC.webroot which makes it possible to properly stub OC.getRootPath() for
testing.
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 23, 2017

Member

Merged all blockers, rebased.

👍 for @felixheidecke's code

Let's get this merged, unless you have objections @felixheidecke. Date picker format stuff to be handled separately as it's a different topic.

Member

PVince81 commented Mar 23, 2017

Merged all blockers, rebased.

👍 for @felixheidecke's code

Let's get this merged, unless you have objections @felixheidecke. Date picker format stuff to be handled separately as it's a different topic.

@PVince81 PVince81 merged commit 6100a3d into master Mar 23, 2017

4 checks passed

Scrutinizer 1 new issues, 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the allow-multiple-link-shares branch Mar 23, 2017

@PVince81 PVince81 restored the allow-multiple-link-shares branch Mar 23, 2017

@PVince81 PVince81 deleted the allow-multiple-link-shares branch Mar 23, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 24, 2017

Member

Cannot unset password when editing, raised here #27481

Member

PVince81 commented Mar 24, 2017

Cannot unset password when editing, raised here #27481

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