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

Add beforeDelete transform #70

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

Conversation

carlbennettnz
Copy link
Contributor

Closes #66

I gave the integration tests a shot, but the first one breaks another test and the second doesn't work at all. How should I be doing this?

@carlbennettnz carlbennettnz force-pushed the before-delete-transform branch from deda53b to bf9fb89 Compare August 31, 2015 00:12
@ethanresnick
Copy link
Owner

Thanks! I don't have much time tonight, but I'll take a look at this tomorrow and let you know.

@carlbennettnz
Copy link
Contributor Author

No problem

@carlbennettnz carlbennettnz force-pushed the before-delete-transform branch from bf9fb89 to 608d5cd Compare August 31, 2015 10:25

// Your route definitions would go here, but if none match...

app.use(function(req, res, next) {
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than putting this example here, can you link to the line with the same code in the example repo? That way, we don't have to keep this code up to date in two places, and (more importantly) people won't get confused about what Front is.

@carlbennettnz
Copy link
Contributor Author

Well the tests pass for me locally :/

@carlbennettnz carlbennettnz deleted the before-delete-transform branch September 10, 2015 20:07
@carlbennettnz carlbennettnz restored the before-delete-transform branch September 10, 2015 20:07
@carlbennettnz carlbennettnz reopened this Sep 10, 2015
@carlbennettnz
Copy link
Contributor Author

I still can't reproduce the problem Circle's facing. If you run these tests locally, do they pass for you?

The id list is deliberately not replaced with the value returned from
the transform as the user should not be able to silently remove ids from
the id list. Therefore, this 'transform' should be used purely as a way
to throw errors when the requested deletion is not allowed.

Closes ethanresnick#66
@carlbennettnz carlbennettnz force-pushed the before-delete-transform branch from e75a991 to 0f5d81d Compare September 10, 2015 20:45
@carlbennettnz
Copy link
Contributor Author

Any chance we could get this fixed and merged within the next few days? I'm almost ready to switch my company's app over to JSON API and I would rather use an official version of this library if possible.

@ethanresnick
Copy link
Owner

I'm almost ready to switch my company's app over to JSON API

Awesome!

Any chance we could get this fixed and merged within the next few days?

I'll take a look at it later this week or over the weekend. Basically, this hasnt beee merged yet only because it feels like we need to solve #28 first, and better account for all the different types of delete requests.

@carlbennettnz
Copy link
Contributor Author

Any update on this?

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

Successfully merging this pull request may close these issues.

2 participants