send all invitations

[dartcafe]

resolves #1304

• Resolve contact groups and circles,
• send invitations to all shares, that did not have an invitation so far

also refactor/maintenance:

• unify sending results

[dartcafe]

@PhrozenByte Could you have a look at the tests. I changed some methods, but the tests fail now due to changed attributes and return values. Would be a big help.

[PhrozenByte]

You changed the parameters of ShareService::sendInvitation(), just change the parameters in the tests as well.

Add use (…, &$expectedShares) (first define $expectedShares as empty array) to this callback and add the mocked Share objects (the result of $this->createShareMock()) to the $expectedShares variable, then change this test to check for $expectedShares instead of $expectedInvitationShareTokens. You can then remove this code.

For RemoveTest it's even simpler, you replace this code by adding the mocked Share objects (again the result of $this->createShareMock()) to a new $expectedShares array and then use this variable for this check instead.

This should do the trick. Unfortunately I don't have a dev setup for the polls app anymore.

[dartcafe]

Something I thought regarding the shareMocks, but I got stuck while trying to understand it. Thanks. I give it a try. I am not familiar with PHPUnit, so... Maybe I will learn something. ;-)

The main problem is, I can't get it to work with my Windows machine, which is my default dev env.

[dartcafe]

@PhrozenByte I suspect the expected return is no more an array, but an object (of OCA\Polls\Model\SentResult). How can I fix the test to match the expected value (or better define the object to be expected).

Did I understand your suggestions right (see last commit)?

[PhrozenByte]

Ah, sorry, my bad, I missed that this logic can't work. You did everything as suggested. The problem is that our mocked ShareService:add() (lines 116-127) is called after we set our expectations for ShareService::sendInvitation() (line 129-132), thus $expectedShares is empty (i.e. we tell PHPUnit that we don't expect any parameter). The return value of ShareService::sendInvitation() isn't checked in the test, because the CLI commands don't rely on the return value.

We can't rely on ShareService:add() to create the mocked Share objects for us, we must create them ourselves first, like the following:

$mockedShares = [];
$expectedShares = [];
foreach ($pollData['expectedInvitations'] ?? [] as $type => $shares) {
	foreach ($shares as $userId) {
		if (!in_array($userId, $pollData['initialShares'][$type] ?? [])) {
			$share = $this->createShareMock($pollData['pollId'], $type, $userId);
			$mockedShares[$type][$userId] = $share;
			$expectedShares[] = $share;
		}
	}
}

And then change our mocked ShareService:add() to return the matching mocked Share object from $mockedShares instead:

			->willReturnCallback(function (int $pollId, string $type, string $userId = '') use ($pollData, $mockedShares): Share {
				$userIdConstraint = $this->logicalOr(...$pollData['expectedShares'][$type] ?? []);
				$userIdConstraint->evaluate($userId);

				if (in_array($userId, $pollData['initialShares'][$type] ?? [])) {
					throw new ShareAlreadyExistsException();
				}

				return $mockedShares[$type][$userId];
			});

The suggested changes for RemoveTest should work. I hope so at least 🙈 😆

[dartcafe]

Hey. Thanks. Helped me for understanding these tests. I adoped your change, see, if it runs through now. 👍

[dartcafe]

@PhrozenByte I am not sure, if I just created an always true condition. But I changed the input parameters to $initialShares and the test runs through. Does it look valid?

[PhrozenByte]

Switching the RemoveTest to $initialShares would indeed always yield true, because you'd allow any of the existing shares to be removed, not just the ones you actually want to remove (consider we have shares ABCD and you want to remove shares AD, using $initialShares accepts any of ABCD, not just AD). You need the following:

		$initialShares = [];
		$expectedShares = [];
		foreach ($pollData['initialShares'] ?? [] as $type => $shares) {
			foreach ($shares as $userId) {
				$share = $this->createShareMock($pollData['pollId'], $type, $userId);
				$initialShares[] = $share;

				if (in_array($userId, $pollData['expectedShares'][$type] ?? [])) {
					$expectedShares[] = $share;
				}
			}
		}

Replace $expectedShareCount by count($expectedShares) and $expectedShareTokens by expectedShares below.

On a side note, our tests (both in RemoveTest, nor AddTest btw) aren't perfect. Right now we only test whether the shares passed to ShareService::add() and ShareService::delete() match any of the expected shares (->with($this->logicalOr(...$expectedShares)) in RemoveTest) and that the number of calls is correct (->expects($this->exactly(count($expectedShares))) in RemoveTest). Thus the test would still pass if one share is passed twice and another share is left out. I don't believe that this is a big issue, but if you want to improve it and actually test whether all expected shares are passed exactly once, you'd need to write a custom check function with ->with(self::callback(function (Share $share): bool { /* body */ })).

[dartcafe]

Argh I was confused, but was the source by myself. I pushed $userId to the $expectedShares array instead of the mockedShare here: 53948c9. 🙈

[dartcafe]

On a side note, our tests (both in RemoveTest, nor AddTest btw) aren't perfect.

At least there are tests. 😆 The rest of the project would need a full test suite, but I have no time for that. Since I know the structure of this app, I can test most of it manually. But one day ... who knows 😉

[PhrozenByte]

Nice, tests pass now. Great work, LGTM! 👍

At least there are tests. 😆 The rest of the project would need a full test suite, but I have no time for that. Since I know the structure of this app, I can test most of it manually. But one day ... who knows 😉

I know the struggle 🙈 Creating tests isn't my favourite job either. To be honest, if it's mostly a single developer project and this developer has a complete overview over the whole project (like you do), and if the project is no library that is used by other developers (like this app), one can argue that tests don't provide a major advantage. Sure, they undoubtedly improve things and are much appreciated, but writing them takes a lot of effort and time, which is often better spent in developing new features. This only changes - but then drastically - the more developers are working on the project. But if that's not the case I don't see much of a problem when there is no test suite...

[dartcafe]

Thanks again. I'll merge for the new beta