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
| // 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?
There was a problem hiding this comment.
@konradoboza No, there is no removal API — see https://github.com/ibexa/user/blob/5.0/src/lib/Invitation/Persistence/DoctrineGateway.php, so rows are in fact never deleted from the invitation tables at all: accepted, expired and stale invitations all stay forever.
I could add a deleteInvitation() to Persistence\Handler + DoctrineGateway but @bnowak suggested adding DAMA to this package separately, so the need for that api call would be short-lived. What do you prefer?
There was a problem hiding this comment.
I suggest I leave the code as is, and that I makes a follow-up PR with DAMA
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: