Skip to content

Conversation

@nikaro
Copy link
Contributor

@nikaro nikaro commented Mar 1, 2017

Hello, i needed these functions so here they are.

@mention-bot
Copy link

@nikaro, thanks for your PR! By analyzing the history of the files in this pull request, we identified @soalhn, @PVince81 and @individual-it to be potential reviewers.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Great stuff!

Apart from the duplicate code from get_users this looks good.


raise HTTPResponseError(res)

def get_users(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the existing method "search_users" already does that but it requires an additional argument.

I suggest you rewire it with "get_users" to avoid duplicate code.
Maybe rewrite get_users to call search_users without argument, and make search_users accept the empty argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PVince81 what do you think about this :

    def search_users(self, user_name=''):
        """Searches for users via provisioning API.
        If you get back an error 999, then the provisioning API is not enabled.

        :param user_name:  name of user to be searched for
        :returns: list of usernames that contain user_name as substring
        :raises: HTTPResponseError in case an HTTP error status was returned

        """
        res = self._make_ocs_request(
            'GET',
            self.OCS_SERVICE_CLOUD,
            'users?search=' + user_name
        )

        if res.status_code == 200:
            tree = ET.fromstring(res.content)
            users = [x.text for x in tree.findall('data/users/element')]

            return users

        raise HTTPResponseError(res)

    def get_users(self):
        """Get users via provisioning API.
        If you get back an error 999, then the provisioning API is not enabled.

        :returns: list of usernames
        :raises: HTTPResponseError in case an HTTP error status was returned

        """
        return self.search_users()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or i can leave search_users untouched by doing :

    def search_users(self, user_name):
        """Searches for users via provisioning API.
        If you get back an error 999, then the provisioning API is not enabled.

        :param user_name:  name of user to be searched for
        :returns: list of usernames that contain user_name as substring
        :raises: HTTPResponseError in case an HTTP error status was returned

        """
        res = self._make_ocs_request(
            'GET',
            self.OCS_SERVICE_CLOUD,
            'users?search=' + user_name
        )

        if res.status_code == 200:
            tree = ET.fromstring(res.content)
            users = [x.text for x in tree.findall('data/users/element')]

            return users

        raise HTTPResponseError(res)

    def get_users(self):
        """Get users via provisioning API.
        If you get back an error 999, then the provisioning API is not enabled.

        :returns: list of usernames
        :raises: HTTPResponseError in case an HTTP error status was returned

        """
        return self.search_users('')

What do you think is the best ?

  • in the first case get_users is just an alias for empty search_users
  • in the second case, we keep throwing an exception if there is no argument passed `search_users'

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to let search_users accept an empty string.

So let's use your last solution, the one with the code from #187 (comment).

As a bonus, it would be good to discard the "?search=" bit if no search string is passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-)

@PVince81 PVince81 added this to the 0.5 milestone Mar 1, 2017
@PVince81
Copy link
Contributor

PVince81 commented Mar 1, 2017

Great, thanks a lot ! 👍

@PVince81 PVince81 merged commit 1b13076 into owncloud:master Mar 1, 2017
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.

3 participants