Page MenuHomekolab.org

Signup Invitations
ClosedPublic

Authored by machniak on Apr 2 2021, 10:16 AM.

Details

Reviewers
mollekopf
Group Reviewers
Restricted Project
Commits
rKe7a47d5d39d1: Signup Invitations
Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
dev/signup-invitations
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 34051
Build 13546: arc lint + arc unit

Event Timeline

machniak requested review of this revision.Apr 2 2021, 10:16 AM
machniak created this revision.
machniak updated this revision to Diff 6805.Apr 2 2021, 12:04 PM
  • Cleanup

Looks good in general.

Notes:

  • There's no expiry for signup codes, but I suppose that's ok (we have the timestamps, so we can add that later)
  • UUID management could be a bit more generalized I think. Perhaps using a trait: https://dev.to/cverster/uuids-in-laravel-7-4kke. Duplicate detection is not necessary IMO with a UUIDv4 algorithm, the collision probability is low enough and a unique constraint can be added on the database level (for the once in a lifetime event of a collision).
  • I don't think it's a performance critical database table, so I'm not worried about that aspect too much.
src/app/Http/Controllers/API/V4/Reseller/InvitationsController.php
156

Personally I would rather abort if any of the addresses fails. It seems easier to me to provide a valid file than to try after the fact to figure out which ones failed/succeeded.

src/app/SignupInvitation.php
13

It seems this can be removed?

28

This could be shifted by 3 instead, did you leave space on purpose?

50

Can data be removed?

57

Can data be removed?

src/database/migrations/2021_03_26_080000_create_signup_invitations_table.php
21

Is there a reason for not using uuid('id'); ?

24

Remove?

32

Are we using this index?

InvitationsController::index does not seem to use status (but created_at)

src/resources/lang/en/mail.php
79

Probably needs to happen before merge?

src/resources/vue/Reseller/Dashboard.vue
8

I think this isn't here on purpose?

src/resources/vue/Reseller/Invitations.vue
83

As a suggestion:

"To send multiple invitations at once, provide a CSV (comma separated value) file containing one email address per value, or alternatively a plain-text file with one email address per line."

machniak updated this revision to Diff 6997.Apr 27 2021, 12:24 PM
  • Don't process csv if any email address is invalid
  • Cleanup
machniak planned changes to this revision.Apr 27 2021, 12:35 PM
machniak marked 8 inline comments as done.
machniak added inline comments.
src/database/migrations/2021_03_26_080000_create_signup_invitations_table.php
21

I guess we just didn't know it existed ;) Maybe Jeroen would know.

32

Removed. I was just anticipating a future use.

machniak updated this revision to Diff 7006.Apr 27 2021, 12:44 PM
  • Add wording for the invitation email

@mollekopf Ready for the final review.

mollekopf requested changes to this revision.Apr 28 2021, 11:25 AM

Just the final fix, you can then ship it.

src/app/Http/Controllers/API/V4/Reseller/InvitationsController.php
184

Same as above, abort on error.

This revision now requires changes to proceed.Apr 28 2021, 11:25 AM
machniak added inline comments.Apr 28 2021, 11:28 AM
src/app/Http/Controllers/API/V4/Reseller/InvitationsController.php
184

There's no loop here. So, it will abort on error.

mollekopf accepted this revision.Apr 28 2021, 12:38 PM
This revision is now accepted and ready to land.Apr 28 2021, 12:38 PM
machniak closed this revision.Apr 30 2021, 11:21 AM

Merged into dev/reseller.