Invert hide email logic to be possitive by default. #3731
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3731 +/- ##
==========================================
- Coverage 99.01% 99.01% -0.01%
==========================================
Files 271 270 -1
Lines 6197 6189 -8
==========================================
- Hits 6136 6128 -8
Misses 61 61
|
| @@ -0,0 +1,17 @@ | |||
| class HideEmailByDefault < ActiveRecord::Migration[7.0] | |||
There was a problem hiding this comment.
I would prefer we do this in multiple steps:
- add new
public_emailcolumn - dual write both columns
- backfill new column
- read only from new column
- delete old column
otherwise, there is the potential that any mistake will leave us with irrecoverable data changes
There was a problem hiding this comment.
If I understand it well, whole up/down blocks are wrapped in transaction and PostgreSQL schema is mostly transactional. That means whole block is executed (or nothing) leaving no space for unpredictable state. 🤔 But I can change to suggested format if needed.
There was a problem hiding this comment.
I don't feel too strongly but I also prefer to split these kinds of data changes into multiple steps as @segiddins described.
There was a problem hiding this comment.
@segiddins I'm about to merge this. If I understand it well (by recent experience), we will need to do it exactly as you mentioned (using ignored_columns trick) to make smooth deployment. Can you confirm please? I'll update the PR.
There was a problem hiding this comment.
please rebase & see if strong_migrations errors
There was a problem hiding this comment.
It does not by default, since this migration has old timestamp. But setting StrongMigrations.start_after = 0 and trying to run this migration errors thanks to strong_migrations.
simi
force-pushed
the
public-email
branch
3 times, most recently
from
84a69cd
to
a0504bc
Compare
There will be additional PR to remove the column. We can check how that affects auditing feature and what we can do about that to not block unrelated changes (like this) by auditing feature. |
- move from hide_email boolean into public_email - makes email hidden by default - add option to optionally show email in public profile during registration - ignore hide_email column (not used anymore) - remove backfill user public email task (since it relies on hide_email column)
This is motivated by #3287 and is initial pull request in serie. For now my intention is to revert the wording and logic in app. Email should be private by default and user should opt-in to make it public if welcomed.
This is currently true in app logic, but the wording around seems opposite. Also it is not known during registration if email is going to be publicly visible. With this change it should be clear.
In next step I would like to introduce additional checkbox to opt-in into using gravatar to handle #3278 and #3287.
And in the end I would like to inline profile edit form and sign-up form into one partial providing all options and columns in both. IMHO there is big chance to get even optional fields like Twitter handle filled during registration. It seems bad UX to register with just few fields and need to complete profile at profile edit page (since there is currently no way to let new user know profile could be completed later).
In the end I would like to revisit some users columns and add
NOT NULLconstraint (for example topublic_emailone), to prevent potential 3 states on boolean column.new checkbox in sign up form
/cc @damien @jchestershopify