[WIP] Fixed group share members search for mail notifications #12030

popov-d · 2014-11-07T12:25:08Z

Issue #11808, my changes described there yesterday.

multiple group. Issue #11808.

ghost · 2014-11-07T12:28:03Z

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

LukasReschke · 2014-11-07T12:32:06Z

@nickvergessen Care to review this? Thanks!

LukasReschke · 2014-11-07T12:33:36Z

@owncloud-bot This is okay to test.

LukasReschke · 2014-11-07T12:33:40Z

@owncloud-bot Retest this please.

ghost · 2014-11-07T13:15:26Z

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

Build result: FAILURE

[...truncated 10 lines...] > git config remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Cleaning workspace > git rev-parse --verify HEAD # timeout=10No valid HEAD. Skipping the resetting > git clean -fdx # timeout=10Fetching upstream changes from https://github.com/owncloud/core.git > git fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git rev-parse origin/pr/12030/merge^{commit} # timeout=10Checking out Revision 2521a5f53c49ad5045b4cb3b0ff5913dfad55229 (detached) > git config core.sparsecheckout # timeout=10 > git checkout -f 2521a5f53c49ad5045b4cb3b0ff5913dfad55229 > git rev-list 79fbe9f32cf37db2c34d11fd8d23cfd7984c130e # timeout=10 > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » vm-slave-02pull-request-analyser-ng-simple » vm-slave-02 completed with result FAILUREStarted calculate disk usage of buildFinished Calculation of disk usage of build in 0 secondsStarted calculate disk usage of workspaceFinished Calculation of disk usage of workspace in 2 second
💣 Test FAILed. 💣

LukasReschke · 2014-11-07T13:19:03Z

@popov-d Thanks for your contribution. This seems to fail for some unit tests as pointed out at https://ci.owncloud.org//job/pull-request-analyser-ng-simple/2368/label=vm-slave-02/. The SQL query seems also to be wrong according to the Jenkins log.

Can I ask you to take a look at this? - If you need help to setup a proper test environment we can gladly help you with that.
Basically, it's just installing ownCloud and then executing "bash autotest.sh sqlite", all tests will then get executed (can take some time) and the failing tests will be shown to you allowing you to test your changes locally.

popov-d · 2014-11-07T13:33:58Z

It seems that \OC_Group::getUserGroups($user) returns empty list. That can't happen when this method is called for a mail group notification, but in current version it is called also from other places. I need another commit. Should I make another pull request when it is ready?

LukasReschke · 2014-11-07T13:34:31Z

I need another commit. Should I make another pull request when it is ready?

You can directly commit to your fork and it will appear on this pull request automatically. That's the cool thing about them :-)

Avoid searching for empty group list in getItemSharedWithUser(). Broke tests in previous commit, #12030.

popov-d · 2014-11-07T13:59:21Z

Here is the commit. When group list is empty, calling group query is useless. Just avoided it instead of making an SQL error.

nickvergessen · 2014-11-07T14:34:06Z

lib/private/share/share.php

-						WHERE
-						`' . $column . '` = ? AND `item_type` = ? AND `share_with` in (?)'
-					);
+			if(count($groups) > 0) {


Just use if (!empty($groups)) { instead

popov-d · 2014-11-07T14:43:58Z

Sorry, I am not a PHP expert. A little more used with Perl. Changed.

scrutinizer-notifier · 2014-11-07T15:42:34Z

The inspection completed: 2 updated code elements

ghost · 2014-11-21T14:59:01Z

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

popov-d · 2014-12-01T10:41:10Z

A question about previous note from owncloud-bot. I have already signed and sent the agreeemet when making first version of pull request. Should I repeat it or that version is enough?

LukasReschke · 2014-12-01T10:41:45Z

@karlitschek Can you confirm?

popov-d · 2014-12-01T10:49:23Z

I sent mail with the signed scan to frank@owncloud.com, as was suggested. From popov@krista.ru at 07.11.2014 16:12 MSK(+03). I can re-send it once more if needed, may be to another address?

karlitschek · 2014-12-01T13:38:40Z

@popov-d Sorry. Could you email it again?

popov-d · 2014-12-01T13:47:29Z

I sent it again to the same address.

karlitschek · 2014-12-01T14:32:33Z

@LukasReschke confirmed

LukasReschke · 2014-12-01T14:33:21Z

@owncloud-bot Retest this please.

ghost · 2014-12-01T15:33:55Z

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

LukasReschke · 2015-02-24T11:43:12Z

@popov-d Sorry for the delay on this one. Could you rebase this PR so we can finally merge it? Thanks! 😄

popov-d · 2015-02-24T13:07:46Z

I don't think it's needed anymore. There was a slightly different change in the same place in 7.0.3 or 7.0.4 that also fixed the problem. It worked for me since that update and works now in 8.0, without any patching. Don't know who did it, but I think you better thank that person.

LukasReschke · 2015-02-24T13:15:20Z

Thanks for the feedback. Closing this one then 😄

#24800) * Fixed group share searching for members of multiple group. Issue #11808. * Fixed group share searching, continued. Avoid searching for empty group list in getItemSharedWithUser(). Broke tests in previous commit, #12030. * Simler check for group count. * Fix for #24783 , described there * Now it's #24272, 24783 was a duplicate. Previous change was also not very good. Now we don't create ZIP with a single file inside.

Fixed group share searching for members of

034c355

multiple group. Issue #11808.

LukasReschke added the 3 - To Review label Nov 7, 2014

popov-d mentioned this pull request Nov 7, 2014

owncloud sharing notification emails without shared resource #11808

Closed

Fixed group share searching, continued.

8a6dedb

Avoid searching for empty group list in getItemSharedWithUser(). Broke tests in previous commit, #12030.

nickvergessen reviewed Nov 7, 2014
View reviewed changes

Simler check for group count.

c602c7f

LukasReschke added 2 - Developing and removed 3 - To Review labels Feb 24, 2015

LukasReschke changed the title ~~Fixed group share members search for mail notifications~~ [WIP] Fixed group share members search for mail notifications Feb 24, 2015

LukasReschke closed this Feb 24, 2015

lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fixed group share members search for mail notifications #12030

[WIP] Fixed group share members search for mail notifications #12030

popov-d commented Nov 7, 2014

ghost commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

ghost commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

popov-d commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

popov-d commented Nov 7, 2014

nickvergessen Nov 7, 2014

popov-d commented Nov 7, 2014

scrutinizer-notifier commented Nov 7, 2014

ghost commented Nov 21, 2014

popov-d commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

popov-d commented Dec 1, 2014

karlitschek commented Dec 1, 2014

popov-d commented Dec 1, 2014

karlitschek commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

ghost commented Dec 1, 2014

LukasReschke commented Feb 24, 2015

popov-d commented Feb 24, 2015

LukasReschke commented Feb 24, 2015

[WIP] Fixed group share members search for mail notifications #12030

[WIP] Fixed group share members search for mail notifications #12030

Conversation

popov-d commented Nov 7, 2014

ghost commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

ghost commented Nov 7, 2014

Build result: FAILURE

LukasReschke commented Nov 7, 2014

popov-d commented Nov 7, 2014

LukasReschke commented Nov 7, 2014

popov-d commented Nov 7, 2014

nickvergessen Nov 7, 2014

Choose a reason for hiding this comment

popov-d commented Nov 7, 2014

scrutinizer-notifier commented Nov 7, 2014

ghost commented Nov 21, 2014

popov-d commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

popov-d commented Dec 1, 2014

karlitschek commented Dec 1, 2014

popov-d commented Dec 1, 2014

karlitschek commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

ghost commented Dec 1, 2014

LukasReschke commented Feb 24, 2015

popov-d commented Feb 24, 2015

LukasReschke commented Feb 24, 2015