apsislabs / phi_attrs

HIPAA compliant PHI access logging for Ruby on Rails.
MIT License
27 stars 3 forks source link

Combine various `allow_phi[!]` methods #19

Closed HenryKeiter closed 5 years ago

HenryKeiter commented 6 years ago

I don't think there's any reason to name the block-syntax method (allow_phi) differently from the standard use (allow_phi!). We can simply check for the existence of a block at the top. This would reduce the likelihood of doing the wrong thing by accidentally calling the wrong method.

For example, the following is currently perfectly good-looking code:

Foo.allow_phi!('henry', 'making a mistake') do
  puts Foo.first.phi_field
end

The issue is...

The block here is never run, because I accidentally used the bang method, which doesn't support blocks. :man_facepalming:

Crisfole commented 5 years ago

I'm more in favor of throwing an error in that situation. "allow_phi! does not accept blocks! Please use allow_phi"

egreer commented 5 years ago

I agree, I think that the ! methods and blocks should have a different syntax as bang represents a mutation that could affect multiple objects.