[HMA] Implement exchange delete API #1733
Conversation
|
Hi @aokolish! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
| if collab is None: | ||
| return {"message": "success"} | ||
| abort(501, "Not yet implemented") | ||
| abort(404, f"exchange '{exchange_name}' not found") |
There was a problem hiding this comment.
blocking: I think we debated this when we originally implemented it, and thought it was less confusing to say that deleting something that doesn't exist is success, so you can naively retry errors.
All the other deletions (e.g. bank delete) follow this pattern as well, so I'm hesitant to add exceptions unless I can understand more about the case.
Did you run into a condition that led you to conclude to change it, or is this just a taste change?
There was a problem hiding this comment.
Ah, that's ok with me.
I was following what I thought was the established pattern. I see it in bank_delete_content for example. However, the rest of the APIs that 404 on things not being found are for updates.
aokolish
changed the title
|
@aokolish - thanks a ton for contributing to HMA! Let me know if the CLA is a hard blocker - I can merge when it's complete, or else tell me to take over. |
|
@aokolish - looks like you just need to run the |
aokolish
force-pushed
the
ao-exchange-delete
branch
from
f85d6b4 to
91db4f2
Compare
Summary
This PR implements the exchange delete API which previously would just return a 501 when called.
Test Plan
Unit tests + I manually tested this in my local HMA dev env & UI.