Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pilaf opened this issue Jun 17, 2019 · 3 comments
Open

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

pilaf opened this issue Jun 17, 2019 · 3 comments
Labels
bug Something isn't working

Comments

@pilaf
Copy link
Collaborator

pilaf commented Jun 17, 2019

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.

@pilaf pilaf added the bug Something isn't working label Jun 17, 2019
@turino
Copy link
Contributor

turino commented Jul 12, 2019

@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.

  1. 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
Copy link
Collaborator Author

pilaf commented Jul 15, 2019

@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 pilaf changed the title FmRest::Spyke::Base#delete should return a boolean FmRest::Spyke::Base#destroy should return a boolean Jul 15, 2019
@pilaf
Copy link
Collaborator Author

pilaf commented Jul 15, 2019

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants