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.

Sign up for GitHub

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

Jump to bottom

Add pagination/search through REPORT #27

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Add pagination/search through REPORT #27

merged 2 commits into from
Mar 1, 2017

Conversation

PVince81
Copy link
Contributor

Fixes #16

Example report:

<?xml version="1.0" encoding="utf-8" ?>
<oc:search-query xmlns:a="DAV:" xmlns:oc="http://owncloud.org/ns">
        <a:prop>
                <oc:role></oc:role>
                <oc:display-name></oc:display-name>
        </a:prop>
        <oc:search>
                <oc:pattern>us</oc:pattern>
                <oc:limit>5</oc:limit>
                <oc:offset>0</oc:offset>
        </oc:search>
</oc:search-query>

You can apply this to the following URLs:

  • groups list: curl -i --data-binary "@report-customgroups.xml" -X REPORT -H "Content-Type: text/xml" http://admin:admin@localhost/owncloud/remote.php/dav/customgroups/groups
  • group members list: curl -i --data-binary "@report-customgroups.xml" -X REPORT -H "Content-Type: text/xml" http://admin:admin@localhost/owncloud/remote.php/dav/customgroups/groups/group1
  • user membership list: curl -i --data-binary "@report-customgroups.xml" -X REPORT -H "Content-Type: text/xml" http://admin:admin@localhost/owncloud/remote.php/dav/customgroups/users/user1/

@DeepDiver1975 @jvillafanez please review.

If the implementation style is ok I'll add matching unit tests.

  • unit tests

@PVince81 PVince81 added this to the 10.0 milestone Feb 23, 2017
@PVince81 PVince81 mentioned this pull request Feb 23, 2017
Closed
@PVince81 PVince81 force-pushed the report-pagination branch 2 times, most recently from 6e6e790 to 58f7ef4 Compare February 27, 2017 10:37
@PVince81
Copy link
Contributor Author

Do we agree on the general approach so I can solidify this with unit tests ?

jvillafanez
jvillafanez reviewed Feb 28, 2017
View reviewed changes
lib/CustomGroupsDatabaseHandler.php Outdated
public function getGroups() {
return $this->searchGroups();
public function getGroups($pattern = null, $offset = 0, $limit = -1) {
return $this->searchGroups($pattern, $offset, $limit);
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be changed

lib/CustomGroupsDatabaseHandler.php Outdated
if ($search !== null) {
if ($search->getPattern() !== null && $property !== null) {
$likeString = '%' . $this->dbConn->escapeLikeParameter(strtolower($search->getPattern())) . '%';
$qb->andWhere($qb->expr()->like($qb->createFunction('LOWER(`' . $property . '`)'), $qb->createNamedParameter($likeString)));
Copy link
Member

Choose a reason for hiding this comment

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

that lower function is strictly needed?. Although it might not be risky, the behaviour isn't optional and isn't documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how we did search in the default group backend. I think it makes sense to keep it.

lib/CustomGroupsDatabaseHandler.php Outdated
* @return array an array of member info
* @throws \Doctrine\DBAL\Exception\DriverException in case of database exception
*/
public function getUserMemberships($uid, $roleFilter = null) {
public function getUserMemberships($uid, $roleFilter = null, Search $search = null) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we can switch the roleFilter and search parameters at this point. I've seen too much getUserMembership(uid, null, search). A getUserMembership(uid, search) with the roleFilter as null by default looks better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also hesitated to maybe put the role filter as a member of the Search class, what do you think ?

@jvillafanez
Copy link
Member

The approach looks good to me.

@PVince81
Copy link
Contributor Author

@jvillafanez thanks a lot for your feedback !

I have the feeling that having a Search object is the way to go in the future for any paginated queries as it saves having a lot of arguments.

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 28, 2017 •
edited
Loading

  • fix DB handler unit tests
  • add tests for REPORT
  • include roleFilter in the Search class and REPORT args ?

@PVince81
Copy link
Contributor Author

PVince81 commented Feb 28, 2017 •
edited
Loading

  • unit tests for ->search() method on nodes

@PVince81
Copy link
Contributor Author

Ok, I'll try adding roleFilter into the Search object. It should work in most queries.

@PVince81 PVince81 mentioned this pull request Feb 28, 2017
Closed
@PVince81 PVince81 force-pushed the report-pagination branch 2 times, most recently from e31f275 to 7ddc303 Compare February 28, 2017 18:48
@PVince81
Copy link
Contributor Author

This is ready for review now

@PVince81
Copy link
Contributor Author

Unit tests added + a few tweaks

@PVince81 PVince81 mentioned this pull request Feb 28, 2017
Merged
12 tasks
jvillafanez
jvillafanez reviewed Mar 1, 2017
View reviewed changes
lib/CustomGroupsBackend.php Outdated
@@ -79,7 +79,7 @@ public function inGroup($uid, $gid) {
* @return array an array of group names
*/
public function getUserGroups($uid) {
$memberInfos = $this->handler->getUserMemberships($uid, null);
$memberInfos = $this->handler->getUserMemberships($uid, null, null);
Copy link
Member

Choose a reason for hiding this comment

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

too many nulls here

lib/Dav/GroupMembershipCollection.php Outdated
$groupId = $this->groupInfo['group_id'];
if (!$this->helper->isUserMember($groupId)
&& !$this->helper->isUserAdmin($groupId)) {
throw new Forbidden("No permission to list members of group \"$groupId\"");
}
$members = $this->groupsHandler->getGroupMembers($groupId);
$members = $this->groupsHandler->getGroupMembers($groupId, null, $search);
Copy link
Member

Choose a reason for hiding this comment

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

are the parameters correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh oh... why didn't the tests explode... I'll recheck

@PVince81 PVince81 force-pushed the report-pagination branch from 7ddc303 to fdab5a5 Compare March 1, 2017 08:40
@PVince81
Copy link
Contributor Author

PVince81 commented Mar 1, 2017

@jvillafanez fixed now...

The reason why tests passed is because the expectation was also wrong...

@jvillafanez
Copy link
Member

👍

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 1, 2017

and now Travis is imitating Jenkins it seems. It's been an hour and I still can't see any results (loading forever)

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 1, 2017 

    /home/travis/build/owncloud/core/apps/customgroups/tests/integration/custom_groups_features/CustomGroups.feature:147

    /home/travis/build/owncloud/core/apps/customgroups/tests/integration/custom_groups_features/CustomGroups.feature:159

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 1, 2017

Going to merge this anyway now and resolve conflicts and integration tests as part of #26

@PVince81 PVince81 merged commit 3f66a22 into master Mar 1, 2017
@PVince81 PVince81 deleted the report-pagination branch March 1, 2017 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvillafanez jvillafanez jvillafanez left review comments

Assignees
No one assigned
Labels
3 - To Review
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@PVince81 @jvillafanez