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

Share dialog use OCS API #21860

Merged
merged 2 commits into from
Jan 28, 2016
Merged

Share dialog use OCS API #21860

merged 2 commits into from
Jan 28, 2016

Conversation

PVince81
Copy link
Contributor

Preliminary work.

  • create share
  • update share (permissions)
  • delete share
  • enable/disable link share
  • send mail checkbox
  • update link share (kind of clumsy currently due to mismatch property names...)
    • password
    • expiration date ("expireDate" vs "expiration" 😵)
    • public upload
    • mail flag => skipped: no OCS API for now ⚠️
  • mailsend => skipped: no OCS API for now ⚠️
  • reshares
  • remote shares => API broken ?!
  • "collection" mode for some shares, possibly containing the parent share => obsolete with new resharing behavior
  • status indicator in file list
  • retest: enforce password
  • retest: enforce default date

If possible:

  • get rid of OC.Share.loadItem if po => to be done separately due to API complications
  • get rid of legacy structures (OC.Share.currentShares) => to be done separately due to API complications

I'm not 100% on using backbone here because the API is not REST-y enough.
We'll see.

@rullzer FYI

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @blizzz, @DeepDiver1975 and @MTGap to be potential reviewers

@PVince81 PVince81 added this to the 9.0-current milestone Jan 22, 2016
@PVince81
Copy link
Contributor Author

Ref: #21554

@PVince81
Copy link
Contributor Author

I removed the backbone bits for now and used direct ajax calls.

@PVince81
Copy link
Contributor Author

  • TODO: only fetch owner/reshare info once in model.fetch, no need to refetch it as it won't change

@PVince81
Copy link
Contributor Author

  • TODO: try and reduce the number of re-fetches in situations where it's not needed (ex: deleting or changing permissions)

@PVince81
Copy link
Contributor Author

  • BUG: cannot change permissions of a reshare, getting 404

@rullzer bug ?

@PVince81 PVince81 force-pushed the sharedialog-ocs-adapter branch from ada7d32 to 702e567 Compare January 25, 2016 17:16
@PVince81
Copy link
Contributor Author

Okay, I fixed many things including the unit tests, plus a few bonus unit tests.

It's already a nice stable ground.
However due to possible backend bugs this cannot be merged yet.
Let's wait for @rullzer's other OCS API PRs to be merged first.

Also I'd like to try to get rid of the legacy share structure completely, the "OC.Share.loadItem" and "OC.Share.statuses" which is still used by the files app to show share status.

@PVince81
Copy link
Contributor Author

  • replace OC.Share.loadIcons with a call with subfiles=true: curl -X GET 'http://root:admin@localhost/owncloud/ocs/v2.php/apps/files_sharing/api/v1/shares?path=abc&subfiles=true&format=json'
    Note that this is limited to a single level, so need to do this call every time when switching folders, which is fine and will fix Files or folders are not shown as shared unless the page is refreshed #6813
    => to be done separately due to API complications

@PVince81
Copy link
Contributor Author

on the other hand that calls seems wasteful.
If a folder "/abc/test" is shared with 100 users, and the user visits "abc", then we'd receive 100 share entries, just to be able to show the "Shared" icon. Ideal would be to be able to get a summary.

@PVince81
Copy link
Contributor Author

Changing reshare permissions as recieiver now works.

  • BUG: Share with link doesn't work any more: the backend doesn't allow "expireDate" to be an empty string at creation time.

and as previously said, remote shares still cannot be created.

@PVince81 PVince81 force-pushed the sharedialog-ocs-adapter branch from 702e567 to f8aa0f5 Compare January 27, 2016 13:05
@PVince81
Copy link
Contributor Author

Rebased on master

@PVince81
Copy link
Contributor Author

@rullzer with #21960 it works better.

However it doesn't let me "PUT permissions" on a share link to set "Allow upload". It used to work.
I don't want to use "publicUpload"...

@rullzer
Copy link
Contributor

rullzer commented Jan 27, 2016

Aaah. OK I will fix that as well :-)

@PVince81
Copy link
Contributor Author

  • TODO: use urlencoded form instead of JSON for requests, which is supposed to work properly for all calls and will fix the remote share one

@PVince81 PVince81 force-pushed the sharedialog-ocs-adapter branch from 908b0f1 to c1dce9f Compare January 28, 2016 10:03
@PVince81
Copy link
Contributor Author

@MorrisJobke
Copy link
Contributor

Tested and works 👍 (tested together with #21960)

@MorrisJobke
Copy link
Contributor

@PVince81 JS unit tests:

05:04:04 PhantomJS 1.9.8 (Linux 0.0.0) OC.Share.ShareItemModel saveLinkShare creates a new share with default expiration date FAILED
05:04:04    Expected Object({ password: '', passwordChanged: false, permissions: 1, expireDate: '2015-7-23 00:00:00', shareType: 3 }) to equal Object({ password: '', passwordChanged: false, permissions: 1, expireDate: '2015-7-24 00:00:00', shareType: 3 }).
05:04:04        at /ssd/jenkins/workspace/core-ci-linux-jsunit/database/sqlite/label/SLAVE/core/js/tests/specs/shareitemmodelSpec.js:638

@PVince81
Copy link
Contributor Author

Weird, I'm not getting this error locally. Maybe a timezone thing

@PVince81 PVince81 force-pushed the sharedialog-ocs-adapter branch 2 times, most recently from 28b504d to c9f4012 Compare January 28, 2016 11:29
@PVince81
Copy link
Contributor Author

It was a timezone issue.
I've replaced the default expiration date calculation by using momentjs for more accurate UTC calculation: c9f4012

@MorrisJobke
Copy link
Contributor

@PVince81 Please check out the failing unit tests

@PVince81
Copy link
Contributor Author

JS Unit pass now. Let's wait for #21975 to be merged

@rullzer
Copy link
Contributor

rullzer commented Jan 28, 2016

Failing unit tests cased by the merging of the two share pr's... that had some conflicting code... fix is in #21975

Vincent Petry added 2 commits January 28, 2016 15:25
Now using UTC dates with moment js to accurately add the number of days
@PVince81 PVince81 force-pushed the sharedialog-ocs-adapter branch from c9f4012 to 7e1de0e Compare January 28, 2016 14:25
@PVince81
Copy link
Contributor Author

Rebased

@rullzer
Copy link
Contributor

rullzer commented Jan 28, 2016

  • I don't get feedback from the password_policy app if setting the password generates an error

@rullzer
Copy link
Contributor

rullzer commented Jan 28, 2016

Awesome work @PVince81!
Clicked around a lot and beside the password policy app everything worked fine.

I'm fine with fixing the passwordpolicy feedback in a different PR>

👍

PVince81 pushed a commit that referenced this pull request Jan 28, 2016
@PVince81 PVince81 merged commit 8b3d7d0 into master Jan 28, 2016
@PVince81 PVince81 deleted the sharedialog-ocs-adapter branch January 28, 2016 15:56
@PVince81
Copy link
Contributor Author

I'll have a look at that issue separately.

@PVince81
Copy link
Contributor Author

Raised here #21992

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants