Skip to content
This repository has been archived by the owner on Apr 21, 2020. It is now read-only.

Build ka scrub types #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Build ka scrub types #1

wants to merge 4 commits into from

Conversation

kaungst
Copy link

@kaungst kaungst commented Aug 13, 2018

No description provided.

Will not include .idea files.
Adds default scrub_types
- scrub - anonymize field value
- skip - keep field's value the same
- wipe - set field's value to nil
- sterilize - destroy all records
acts_as_scrubbable :first_name, :last_name
acts_as_scrubbable :scrub, :first_name # first_name will be random after `scrub!`
acts_as_scrubbable :skip, :middle_name # middle_name will be original value after `scrub!`
acts_as_scrubbable :wipe, :last_name # last_name will be `nil` after `scrub!`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't love the DSL calling the same method 3 times, but otherwise the concept is sound. is there an easy way to come up with a way to create one dataset for the complete class configuration? maybe something like:

acts_as_scrubbable scrub: [:first_name, lat: :latitude],
                   skip: [:middle_name],
                   wipe: [:last_name]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this technique would need to be paired with some validation that when any fields are configured to be scrubbed, skipped or wiped, that all fields are configured, right? otherwise 'skip' and literally skipping are the same... or is that something AnonCat would do?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, anonymizer_check!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on further thought, it'd be really cool to be able to define scrub procs here, too. e.g.:

acts_as_scrubbable scrub: [gender: -> { %w(M F).sample }]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:wipe scrub type could as easily be a :nullify scrub strategy

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fan of the array update and would push for going more in that direction.

also agree that defining scrub procs would be neat (maybe a well placed is_a?(Proc)?)

went with wipe for the theme :D, also thought it left room for fancier wiping (empty string?). Fine with nullify


class SterilizeExample < ActiveRecord::Base
acts_as_scrubbable :sterilize # table will contain no records after `scrub!`
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

:school => -> { Faker::University.name }
:school => -> { Faker::University.name },
:bs => -> { Faker::Company.bs },
:phone_number => -> { Faker::PhoneNumber.phone_number }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i personally found using these built-ins just some of the time seemed confusing, so didn't use them at all.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after looking at the other PR i see you used :bs a bunch, so makes sense to put it either here or in the initializer config in that case.

def scrub!
return unless self.class.scrubbable?

run_callbacks(:scrub) do
if self.class.sterilizable?
self.destroy!
Copy link

@jakeonfire jakeonfire Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think delete is safer, since the other updates also don't run callbacks, but now that i think about it maybe we do want them in case of dependent: :destroys? dunno...

Copy link

@jakeonfire jakeonfire Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also worried that calling destroy! on every row in a table will be pretty slow. can you think of a way to truncate it instead via SQL a-la the after-hook example in the README?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the tasks.rb:69 there's an initial check for sterilizable models that will call delete_all. you are both correct that it avoids the destroy calls and that there are dependent destroy issues. had realized it running the migration, brought it up in the story. feel like we kind of need the destroy_all b/c of the dependent destroy. side note this also creates an issue for large tables (thinking weight table of 70M records :( )

else
puts "Undefined scrub: #{value} for #{self.class}#{_field}"
end
puts "Undefined scrub: #{value} for #{self.class}.#{_field}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#{self.class}##{_field}" is a bit more ruby-doc

def scrubbable?
false
end

def acts_as_scrubbable(scrub_type=:scrub, *scrubbable_fields, **mapped_fields)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think you can have default values to the left of var-args?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can! it just isn't respected. Was having trouble finding a good way to fit in the scrub_type while not creating too breaking of a change. I think the approach you listed at the top is better (kwargs all the way, arrays of symbols or kwargs as values

...

acts_as_scrubbable :first_name, :last_name
acts_as_scrubbable :scrub, :first_name # first_name will be random after `scrub!`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like "faker-generated" is clearer than "random". i had to look at the source code to figure that out.

...

acts_as_scrubbable :first_name, :last_name
acts_as_scrubbable :scrub, :first_name # first_name will be random after `scrub!`
acts_as_scrubbable :skip, :middle_name # middle_name will be original value after `scrub!`
Copy link

@jakeonfire jakeonfire Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"will be unchanged"?


```ruby
ActsAsScrubbable.configure do |c|
c.add :email_with_prefix, -> { "prefix-#{Faker::Internet.email}" }

c.before_hook do
puts "Running before scrub"
raise "Don't run in production" if Rails.env.production?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will the env ever be production? the other PR assumes it is always development in the anonymizer_check task...
is there a way to instead look at something that is not configurable, like database name, or server, or?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on further thought, this suggestion makes no sense. it's good as-is.

@jakeonfire
Copy link

i'm going to push for getting a simple version out to populate staging, then work on a more unified approach via this branch after.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants