IBX-12340: Pass the required struct option when registering from invitation - #140
IBX-12340: Pass the required struct option when registering from invitation#140vidarl wants to merge 3 commits into
Conversation
| // Fixtures are imported once per run, so without this the invitation survives into | ||
| // InvitationServiceTest, which asserts absolute findInvitations() counts and fails | ||
| $connection = self::getDoctrineConnection(); | ||
| $connection->executeStatement( | ||
| 'DELETE FROM ibexa_user_invitation_assignment WHERE invitation_id IN ' | ||
| . '(SELECT id FROM ibexa_user_invitation WHERE email = :email)', | ||
| ['email' => self::INVITEE_EMAIL] | ||
| ); | ||
| $connection->delete('ibexa_user_invitation', ['email' => self::INVITEE_EMAIL]); |
There was a problem hiding this comment.
What fixture? We are creating an invitation as part of the above test, and DAMADoctrineBundle should cause the transaction to be rolled back after it - this cleanup should not be necessary.
There was a problem hiding this comment.
What fixture?
Sorry, that was bad wording, not really a fixture. The point is that the database is imported only once per run. I have updated the comment in ea9b24f
We are creating an invitation as part of the above test, and DAMADoctrineBundle should cause the transaction to be rolled back after it - this cleanup should not be necessary.
No, there is no dama/doctrine-test-bundle in this package. I had a quick look and it seems we really only use it in the product-catalog* and migrations packages. Other packages misses the step of register the PHPUnit extension, so it stays inactive — see https://github.com/dmaicher/doctrine-test-bundle#using-the-bundle-with-phpunit. So for most Ibexa packages the bundle does nothing unless I am missing something.
As for why this test needs to clean up after itself: the existing InvitationServiceTest asserts absolute row counts, and it also depends on the absence of rollback — testCreateInvitation() creates an invitation and testFindInvitations() then asserts assertCount(1, …). Without the cleanup, three existing tests fails:
- InvitationServiceTest::testFindInvitations
- InvitationServiceTest::testFindInvitationsWithUserGroupFilter
- InvitationServiceTest::testFindInvitationsWithUserGroupAndRoleFilter
So adding DAMA to this package would break those three, and they'd need relative assertions or per-test fixtures first. And fixing this is out-of-scope of this PR IMO
There was a problem hiding this comment.
Actually, we're using DAMA in integration tests of many packages, some examples:
- https://github.com/ibexa/data-intelligence-layer/blob/6.0/phpunit.integration.xml#L21
- https://github.com/ibexa/measurement/blob/5.0/phpunit.integration.xml.dist#L21
- https://github.com/ibexa/activity-log/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/admin-ui/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/core-persistence/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/discounts/blob/6.0/phpunit.integration.xml#L23
- https://github.com/ibexa/fieldtype-page/blob/6.0/phpunit.integration.xml#L23
- https://github.com/ibexa/page-builder/blob/6.0/phpunit-integration.xml#L23
and above are only randomly picked ones.
Also, we have it configured by default in bundle-generator, so I guess that's our convention to use it, rather not using.
I agree adding DAMA is out of scope for this PR, but we should add it separately to avoid manual cleanup after each test that touches db data (DAMA will handle that automatically). If InvitationServiceTest has dependent tests - it means they're invalid tests in fact, as each test should be independent of others. For instance, we couldn't run them in parallel (ofc out of scope) and the order of tests execution matters (when it shouldn't). Maybe DAMA would support phpunit's Depends annotation/attribute, which would make it easy to refactor them. However if not, I guess we should rewrite dependent tests into independent ones.
…uildsForm() removes its invitation
| self::getContainer()->set(UserDispatcher::class, new UserDispatcher()); | ||
| } | ||
|
|
||
| public function testRegisterFromInvitationBuildsForm(): void |
There was a problem hiding this comment.
What we're testing in here, especially? Is that form built without exceptions?
If that's controller test, why can't we test that it on http layer, with using some test http client (calling specific endpoint with expected response code 200)? That would cover "not-throwing exception" expectations and would be more e2e approach 😉
There was a problem hiding this comment.
@bnowak
I was honestly in doubt how to make a test for it. Was considering a behat test, but opted for this approach.
But your suggestion is good and turned out to be quite easy as the test kernel already imports the bundle routing (tests/integration/Resources/routing.yaml)
Fixed in ea4cd83, please have a look
There was a problem hiding this comment.
looks good, thanks for adjusting 👍
| // Fixtures are imported once per run, so without this the invitation survives into | ||
| // InvitationServiceTest, which asserts absolute findInvitations() counts and fails | ||
| $connection = self::getDoctrineConnection(); | ||
| $connection->executeStatement( | ||
| 'DELETE FROM ibexa_user_invitation_assignment WHERE invitation_id IN ' | ||
| . '(SELECT id FROM ibexa_user_invitation WHERE email = :email)', | ||
| ['email' => self::INVITEE_EMAIL] | ||
| ); | ||
| $connection->delete('ibexa_user_invitation', ['email' => self::INVITEE_EMAIL]); |
There was a problem hiding this comment.
Actually, we're using DAMA in integration tests of many packages, some examples:
- https://github.com/ibexa/data-intelligence-layer/blob/6.0/phpunit.integration.xml#L21
- https://github.com/ibexa/measurement/blob/5.0/phpunit.integration.xml.dist#L21
- https://github.com/ibexa/activity-log/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/admin-ui/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/core-persistence/blob/6.0/phpunit.integration.xml#L20
- https://github.com/ibexa/discounts/blob/6.0/phpunit.integration.xml#L23
- https://github.com/ibexa/fieldtype-page/blob/6.0/phpunit.integration.xml#L23
- https://github.com/ibexa/page-builder/blob/6.0/phpunit-integration.xml#L23
and above are only randomly picked ones.
Also, we have it configured by default in bundle-generator, so I guess that's our convention to use it, rather not using.
I agree adding DAMA is out of scope for this PR, but we should add it separately to avoid manual cleanup after each test that touches db data (DAMA will handle that automatically). If InvitationServiceTest has dependent tests - it means they're invalid tests in fact, as each test should be independent of others. For instance, we couldn't run them in parallel (ofc out of scope) and the order of tests execution matters (when it shouldn't). Maybe DAMA would support phpunit's Depends annotation/attribute, which would make it easy to refactor them. However if not, I guess we should rewrite dependent tests into independent ones.
|
| // invitation survives into InvitationServiceTest, which asserts absolute findInvitations() | ||
| // counts and fails | ||
| $connection = self::getDoctrineConnection(); | ||
| $connection->executeStatement( |
There was a problem hiding this comment.
Isn't there API that can be used instead of a vanilla SQL query?
6 participants
Related PRs:
Strictly speaking not related, but needed in order to make the invite user functionality work end-to-end (IBX-12339):
Description:
UserRegisterController::registerFromInvitationAction()buildsUserRegisterTypewithout thestructoption, so/from-invite/register/{hash}always returns HTTP 500 and an invitation can never be accepted.UserRegisterType::getParent()isBaseContentType, which declares->setRequired(['languageCode', 'mainLanguageCode', 'struct'])and forwardsstructinto everyfieldsDataentry.registerAction()in the same controller passes'struct' => $data; the invitation variant does not.Added the missing option, matching
registerAction().For QA:
See ticket for how to reproduce
Documentation: