Page MenuHomekolab.org

Signup code refactoring
ClosedPublic

Authored by machniak on Mar 25 2021, 10:00 AM.

Details

Reviewers
vanmeeuwen
Group Reviewers
Restricted Project
Commits
rK8bf0909d311d: Signup code refactoring
Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak requested review of this revision.Mar 25 2021, 10:00 AM
machniak created this revision.
machniak updated this revision to Diff 6754.Mar 25 2021, 4:15 PM
  • Replace pack() with hex2bin()
  • SignupCode refactor
machniak retitled this revision from Throttling signups using an IP address to Signup code refactoring.Mar 25 2021, 4:15 PM
machniak updated this revision to Diff 6757.Mar 25 2021, 4:21 PM
  • Simplify
mollekopf added inline comments.
src/app/Observers/SignupCodeObserver.php
41

Is it save to assume that this is a valid email address here? Otherwise we'll crash below.

Maybe just do the ?? null if it's not guaranteed that email is valid.

60

Same as above.

src/database/migrations/2021_03_25_100000_signup_code_refactor.php
78

This reverse migration will not restore the data column. Perhaps add a FIXME so we have a chance to spot it before executing it.

mollekopf added inline comments.Mar 31 2021, 10:05 AM
src/app/SignupCode.php
11

Shouldn't the documentation include local_part, domain_part even if extracted via observer from email?

machniak planned changes to this revision.Mar 31 2021, 10:07 AM
machniak added inline comments.
src/app/Observers/SignupCodeObserver.php
41

We validate the email in the controller. And as we don't want invalid emails in here, I think an error is what we actually want to happen on an invalid email. So, I would not change this.

src/database/migrations/2021_03_25_100000_signup_code_refactor.php
78

I guess we can add the reverse operation to what we do in up(). I'll add this. Just truncating the table (making existing codes invalid) wouldn't be a big deal too, imo.

machniak updated this revision to Diff 6814.Apr 6 2021, 12:50 PM
  • Add reverse migration, document more SignupCode properties
vanmeeuwen accepted this revision.Apr 8 2021, 1:07 PM
This revision is now accepted and ready to land.Apr 8 2021, 1:07 PM
This revision was automatically updated to reflect the committed changes.