Brett Smith wrote:
- In ArvadosModel#make_log_around,
log.save
should probably be log.save!
so failure to log causes transaction rollback/fail, rather than silently missing logs.
I'm not sure which is stylistically better, but according to the docs I believe the current implementation will cause a rollback. Rails' callback documentation says that any callback can force a rollback by returning false, which log.save
should do if it fails.
I see this:
The whole callback chain is wrapped in a transaction. If any before callback method returns exactly false or raises an exception, the execution chain gets halted and a ROLLBACK is issued; after callbacks can only accomplish that by raising an exception.
But this seems to say "before callbacks" do what you want, "after callbacks" don't do what you want, and "around callbacks" ... uh, might do what you want? (One of us is missing something...)
OTOH, raising an exception is almost certain (ha) to do what you want.
Perhaps the best answer involves a test case, although I can't think offhand of a convenient way for a test to make log.save fail.
- I don't see how we avoid (or test that we avoid) logging the creation of a (user-initiated) Log object
I'll add a test, but if I'm following you right, I think the self.is_a? Log
conditional in make_log_around
covers this.
Ah, indeed, I missed that. I was too busy looking for a skip_around_filter in the Log class. That works.
- New Log semantics (can't update or delete your own voluntarily-added logs) are a surprise, but arguably a good new feature. (Existing permission system would be enough to protect the automatically generated logs.)
Sorry for the surprise. I went there because I had to overwrite permission_to_create
, and then figured overriding the others made sense for consistency's sake, and I didn't fully understand how that hooked into permission to view. If you're okay with this general approach, I'm happy to make the other changes you suggested.
Yes, I think it does make sense. Normally a logging facility is not expected to provide "update log" and "delete log" operations, right?