Page MenuHomePhorge

Signup Invitations
ClosedPublic

Authored by machniak on Apr 2 2021, 10:16 AM.
Tags
None
Referenced Files
F11585583: D2428.id6997.diff
Thu, Mar 28, 12:24 PM
Unknown Object (File)
Sat, Mar 16, 1:26 PM
Unknown Object (File)
Feb 25 2024, 11:34 AM
Unknown Object (File)
Feb 21 2024, 7:20 PM
Unknown Object (File)
Feb 17 2024, 6:49 PM
Unknown Object (File)
Feb 11 2024, 4:12 AM
Unknown Object (File)
Feb 6 2024, 7:26 PM
Unknown Object (File)
Feb 1 2024, 4:43 PM
Subscribers
Restricted Project

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 Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 33787
Build 13387: arc lint + arc unit

Event Timeline

machniak created this revision.

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."

  • Don't process csv if any email address is invalid
  • Cleanup
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.

  • Add wording for the invitation email

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
src/app/Http/Controllers/API/V4/Reseller/InvitationsController.php
184

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

This revision is now accepted and ready to land.Apr 28 2021, 12:38 PM