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

Extention of OCS to list the people/groups we can share with #13587

Closed
wants to merge 1 commit into from
Closed

Extention of OCS to list the people/groups we can share with #13587

wants to merge 1 commit into from

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Jan 22, 2015

To allow the client to also do user/group sharing we need autocompletion on user and group names. Implementation is pretty straight forward, I directly wrote unit tests to ensure proper behaviour.

@DeepDiver1975
Copy link
Member

Thanks a lot @rullzer - we will have a look at this as soon as we start with the dev cycle for 8.1.
Current top prio is to get 8.0 out.

@rullzer
Copy link
Contributor Author

rullzer commented Jan 22, 2015

@DeepDiver1975 Yeah I figured that. I'll try to fix all the warnings that are autodetected here.

Also I do not get why one of the unit tests fails since they work fine on my system. But I'll look into that this evening.

@DeepDiver1975
Copy link
Member

Also I do not get why one of the unit tests fails since they work fine on my system. But I'll look into that this evening.

well - most probably this is related to the environment like which users/groups exist and so on. Our current OCS code is not really ideal for unit testing. We are looking into alternatives for 8.1 - #12454

@@ -596,4 +596,43 @@ private static function getShareFromId($shareID) {

}

public static function shareWith() {
Copy link
Member

Choose a reason for hiding this comment

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

the result set can be really hugh - which requires pagination - please add parameters like $limit and $offset

furthermore clients want to implement type ahead input fields which requires a $search parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes that makes sense! It is added now. However now we still construct the entire array first on the server. So I will improve that later.

Also I need to look up how to test this.

@PVince81
Copy link
Contributor

PVince81 commented Feb 9, 2015

Wow, is that somewhat a duplicate of the code used by the share dropdown ?
In any case, we need to make the share dropdown based on OCS API instead of internal ajax (in a future PR)

@rullzer
Copy link
Contributor Author

rullzer commented Feb 9, 2015

@PVince81 yeah it is very similar.

@rullzer
Copy link
Contributor Author

rullzer commented Feb 10, 2015

@PVince81 it should not be hard to extend this to also accept and output the current calls of the javascript calls. However I agree that should go into a different PR.

@PVince81
Copy link
Contributor

I actually agree with this PR, will try and test it later this week.

@rullzer
Copy link
Contributor Author

rullzer commented Feb 10, 2015

Great, then I'll start working on a patch to convert the ajax call to a call to the OCS API.

@PVince81
Copy link
Contributor

Good luck with that. Hint: messy code.

@DeepDiver1975
Copy link
Member

@rullzer can I ask you to squash your commits? THX

@rullzer
Copy link
Contributor Author

rullzer commented Mar 5, 2015

@DeepDiver1975 squased and rebased

@MorrisJobke
Copy link
Contributor

@DeepDiver1975 @PVince81 @LukasReschke What is the state of this? Ready for review or is there more to add?

@rullzer
Copy link
Contributor Author

rullzer commented Mar 18, 2015

I really want to get this (in one form or another) in 8.1 so I can implement user/group sharing in the client.

I assume the code is "fine" since it is very similar to the ajax code (again the new modal should start using the same endpoint 😉 ).

However what is left to discuss is the endpoint. Adding yet another endpoint here might not be the cleanest solution. Maybe we want something else like .../users and .../groups (somewhat related to #12454). That way at least this part of the API would be more rest like. And we would avoid more future hassel if we want to migrate.

Comment/suggestions?


I'll also squash my commits again and rebase to be sure I do not break anything in the current state.

}
}
if (array_key_exists('search', $_GET)) {
$search = $_GET['search'];
Copy link
Member

Choose a reason for hiding this comment

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

Please cast to (string) to prevent users from passing an array which potentially might result in havoc situations 🙈

* Simple API call to let the client(s) obtain a list of people we can share
files with.
  * More or less the functionality the ajax call already provided but
    then in a OCS call
  * Filtering build in
  * Pagination build in
* Added unit tests
@scrutinizer-notifier
Copy link

The inspection completed: 4 updated code elements

@ghost
Copy link

ghost commented Mar 19, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10627/
Test PASSed.

@rullzer rullzer modified the milestones: 8.1-current, 8.2-next Mar 30, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Mar 30, 2015

I moved this to 8.2 since I think it would be better to do it right in 1 go then to migrate away from it pretty soon already.

@ghost
Copy link

ghost commented Apr 30, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12049/
💣 Test FAILed. 💣

nooo432

@rullzer rullzer mentioned this pull request May 31, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Jul 2, 2015

I'm closing tis PR in favor of the solution I'll be implenting for #16646

@rullzer rullzer closed this Jul 2, 2015
@rullzer rullzer deleted the ocs_listsharewith branch July 2, 2015 15:04
@rullzer rullzer removed this from the 8.2-current-2015-10-15 milestone Jul 7, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 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.

6 participants