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

Properly search for users when limittogroups is enabled #20772

Merged
merged 2 commits into from
Aug 21, 2020

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented May 1, 2020

Searching just for the uid is not enough.
This makes sure this done properly again now.

Actually fixes a valid failure in the integrationtests...

@rullzer rullzer added bug 3. to review Waiting for reviews labels May 1, 2020
@rullzer rullzer added this to the Nextcloud 19 milestone May 1, 2020
@rullzer rullzer mentioned this pull request May 2, 2020
2 tasks
@rullzer
Copy link
Member Author

rullzer commented May 4, 2020

To test

  • limit users to share only in their own groups
  • create a group TestGroup
  • create a user TestUser that belongs to TestGroup
  • create a user TestUser2 that belongs as well to TestGroup

Now as TestUser search the sharee endpoint for 'test'

Before:

  • see the group but not the user

After:

  • see both

@nickvergessen nickvergessen force-pushed the fix/sharee-integration-test branch from 550657d to c9e4e27 Compare May 4, 2020 09:56
@nickvergessen
Copy link
Member

I can not reproduce the failure before the patch so unwilling to approve the fix, but at least it is not breaking something.

@rullzer
Copy link
Member Author

rullzer commented May 4, 2020

Ok let me try to explain. Basically it does boom because of:

$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);

Which goes to:

public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {

Now there are 2 issues here

  1. It doesn't search in displaynames (despite the method names)
  2. it does a regular like. Which is case sensitive.

The new way will actually search displaynames as well. Which is what you want.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Smarter!

@rullzer rullzer mentioned this pull request May 4, 2020
3 tasks
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke

This comment has been minimized.

@rullzer
Copy link
Member Author

rullzer commented May 6, 2020

Ok it seems I just don't get the tests...
@MorrisJobke @nickvergessen could one of you have a look and try?

@rullzer rullzer modified the milestones: Nextcloud 19, Nextcloud 20 May 6, 2020
@rullzer
Copy link
Member Author

rullzer commented May 6, 2020

/backport to stable19

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@MorrisJobke
Copy link
Member

Underlying code was changed with #21452

@MorrisJobke MorrisJobke force-pushed the fix/sharee-integration-test branch from c9e4e27 to 4054c47 Compare August 20, 2020 13:50
@MorrisJobke
Copy link
Member

I merged the changes from this PR with the enhancements from #21452 to also filter out disabled users properly.

Ready for review.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 20, 2020
@MorrisJobke
Copy link
Member

What?

/github/workspace/apps/user_status/lib/Db/UserStatusMapper.php:84:41:error - ImplicitToStringCast: Argument 2 of OCP\DB\QueryBuilder\IExpressionBuilder::notIn expects array<array-key, mixed>|string, OCP\DB\QueryBuilder\IParameter provided with a __toString method

@MorrisJobke
Copy link
Member

Seems that this came in via another PR.

@MorrisJobke MorrisJobke mentioned this pull request Aug 20, 2020
@faily-bot
Copy link

faily-bot bot commented Aug 20, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32117: failure

sqlite

Show full log
There was 1 error:

1) OCA\Files_Sharing\Tests\SharedMountTest::testPermissionMovedGroupShare with data set #20 ('folder', 1, 11)
Error: Call to a member function getMountPoint() on null

/drone/src/lib/private/Share20/Manager.php:304
/drone/src/lib/private/Share20/Manager.php:705
/drone/src/apps/files_sharing/tests/TestCase.php:255
/drone/src/apps/files_sharing/tests/SharedMountTest.php:355

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

mysql8.0-php7.2

  • cancelled - typically means that the tests took longer than the drone CI allows them to run

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
  Scenario: unmarking a file as favorite causes the file list to be sorted again                          # /drone/src/tests/acceptance/features/app-files.feature:262
    Given I am logged in                                                                                  # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "A name alphabetically lower than welcome.txt"                        # FileListContext::iCreateANewFolderNamed()
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    And I close the details view                                                                          # FilesAppContext::iCloseTheDetailsView()
    And I see that the details view is closed                                                             # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
    And I mark "welcome.txt" as favorite                                                                  # FileListContext::iMarkAsFavorite()
    And I see that "welcome.txt" is marked as favorite                                                    # FileListContext::iSeeThatIsMarkedAsFavorite()
    And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    When I unmark "welcome.txt" as favorite                                                               # FileListContext::iUnmarkAsFavorite()
    Then I see that "welcome.txt" is not marked as favorite                                               # FileListContext::iSeeThatIsNotMarkedAsFavorite()
      Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

@MorrisJobke
Copy link
Member

Seems that this came in via another PR.

It isn't on master :/

@MorrisJobke
Copy link
Member

@kesselb Any idea?

@kesselb
Copy link
Contributor

kesselb commented Aug 20, 2020

@MorrisJobke similar to #22257. But warning is unrelated to this pr.

@rullzer rullzer mentioned this pull request Aug 21, 2020
19 tasks
@MorrisJobke
Copy link
Member

@MorrisJobke similar to #22257. But warning is unrelated to this pr.

Unfortunately this doesn't show up locally :/

rullzer and others added 2 commits August 21, 2020 13:14
Searching just for the uid is not enough.
This makes sure this done properly again now.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
…ed to contain more results

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke force-pushed the fix/sharee-integration-test branch 2 times, most recently from 4c9e8d5 to a0cbf16 Compare August 21, 2020 11:18
@MorrisJobke
Copy link
Member

Actually it was fixed in master -> merging then.

@MorrisJobke MorrisJobke merged commit 8261d7e into master Aug 21, 2020
@MorrisJobke MorrisJobke deleted the fix/sharee-integration-test branch August 21, 2020 11:18
@backportbot-nextcloud
Copy link

The backport to test failed. Please do this backport manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants