'anonymizes user.username'

This introduces knowledge of UserGeo into the User model. Are we okay with that?

The cross-model awareness seems fine to me, especially since they're related, but why don't we set this up as an association?

Something like:

has_many :user_geos, -> {order(updated_at: :desc)}
def latest_user_geo
  user_geos.first
end

That seems great. I made an assumption that @asherkach had a reason for not doing this back when he introduced the UserGeo model, something like isolating PII. I wonder if he has any thoughts on this.

My guess is that there wasn't an obvious need for the association, so I didn't add it. Doubtful that it was related to isolating PII, as the association wouldn't have carried to Redshift (where most of the isolation concerns were centered).

Thanks for taking a look!

@poorvasingal @bencodeorg Please take a look at this test file and let me know if it makes any sense to you. I'd like to make these tests as readable to you as possible so we can all be on the same page about what our system actually does at any time.

Other reviewers, I'd particularly like feedback on anything that might make these tests more readable (though I might not make changes until a future PR)

This test against the User model is now redundant, and the behavior it was testing is strictly a subset of my larger test suite.

Converted to a class for testability.

Shouldn't this be DeleteAccountsHelper.new ?

I wrote this static helper for constructing a configured DeleteAccountsHelper while converting it to a class so I could test with stub dependencies.

https://github.com/code-dot-org/code-dot-org/blob/7f05a3eca19d9f5a9620c0f7028153b221ef3ab2/lib/cdo/delete_accounts_helper.rb#L21-L34

Would it be more idiomatic to put default arguments on the constructor?

Ah, for some reason I didn't see this the first time around and thought it was a typo here.

The static helper is fine, but I do think it would be a bit more idiomatic and discoverable if the constructor had default arguments. Your call.

The cross-model awareness seems fine to me, especially since they're related, but why don't we set this up as an association?

Something like:

has_many :user_geos, -> {order(updated_at: :desc)}
def latest_user_geo
  user_geos.first
end

🎉 factories.

Why not make Seattle the default, and add a user association, so a simple create :user_geo will work? All the other combos will still be supported.

One of the kinds of UserGeo records I want to test against is mostly empty - after sign in, we often create a UserGeo record with just a user id and IP address, nothing else. For that reason I'm leaving the :seattle and :sydney traits separate, but I've done a pass cleaning things up.

Note these don't need to be set explicitly in a block. Since they're exposed as attributes, they will work as params to FactoryGirl.create:

user = create :teacher, 
  ops_first_name: 'test_value',
  # ...

TIL! Wasn't sure how serialized_properties would be handled.

@aoby Is it best practice to add this matching association even though I'm not using any of the helpers it generates, or should I omit it because it's "unused?"

I don't know of guidance either way on that one, and I don't have a strong preference. It shouldn't be any extra overhead to add it since they're lazily loaded.

Out of curiosity, why is parent_email nil, and the others empty? Should we be consistent?

Also note that empty string will pass refute_nil so the initial 2 assertions don't necessarily show that email and hashed_email changed.

Kind of a legacy thing - email has not null in our schema even though in practice it's often empty, but parent_email, added later, uses null as its empty value.

I'm in favor of consistency but think it's a concern that should be addressed in its own change, which would include updating these tests.

Thanks for catching the insufficient assertion here!

nit: switch the args: expected first. See https://apidock.com/ruby/Test/Unit/Assertions/assert_equal

same as above.

assert_equal 'clever', user.provider

note: since studio_person is an association on this id, you can also set it directly:

create :teacher, studio_person: studio_person

Since it appears that we call user.reload after every call to purge_user they could be combined.

I'm in favor of something like this, but wasn't sure whether it would read well. Basically, I feel like it's obvious that purge_user affects the db record, but less obvious that the user instance passed in gets mutated.

student = create :student
purge_user student
# Is the 'student' instance mutated here?

This might read better if the mutated object was returned?

student = create :student
student = purge_user student
# Not surprising that 'student' has changed

Or is this just my JavaScript idiom bleeding into Ruby?

Discussed in person, since this helper is private to this file and purge_user student pretty obviously mutates the student when read as English, deduplicating should take priority over concerns about mutating arguments.