search

beezwax / fmrest-ruby

FileMaker Data API client for Ruby with ActiveRecord-like ORM features
MIT License
22 stars 10 forks source link

FmRest::Spyke::Base#destroy should return a boolean #12

Open pilaf opened 5 years ago

pilaf commented 5 years ago

Currently calling .destroy on a record will return a hash of parsed JSON (Spyke's default behavior), but it should return a boolean with success status instead.

turino commented 5 years ago

@pilaf 2 questions:

1) do you know how to get our FM test server to prevent deletion of a record? I looked into referential integrity options, but it seems there's nothing like dependent: :restrict_with_error in FM. I created an account that doesn't have delete privileges, but then when I try to delete I get

FmRest::APIError::AccountError: FileMaker Data API responded with error 200: Record access is denied

which seems like a better response than false in this case.

2) is it OK to leave delete as is and have destroy return a boolean instead? Given how the code is written, I feel this would be a little cleaner/easier to implement. (And shouldn't destroy be the default way to delete a record anyway?)

pilaf commented 5 years ago

@turino

do you know how to get our FM test server to prevent deletion of a record? I looked into referential integrity options, but it seems there's nothing like dependent: :restrict_with_error in FM. I created an account that doesn't have delete privileges, but then when I try to delete I get

I don't, sorry :(

is it OK to leave delete as is and have destroy return a boolean instead? Given how the code is written, I feel this would be a little cleaner/easier to implement. (And shouldn't destroy be the default way to delete a record anyway?)

Ah, yes, good catch. I'll rename the issue.

BTW, it may be a good idea to also freeze the record object after .destroy for consistency with AR's behavior.

I also noticed that if you .destroy a record with .persisted? == false (e.g. if you just instantiated a record with Model.new) it will perform a DELETE request to /:layout/records, which is not the right thing to happen... we should probably prevent the request in that case and just freeze the record instead.

pilaf commented 5 years ago

@turino On second thought, should .destroy return the record object instead? That's what AR does, so if we're aiming for consistency with it...