-
Notifications
You must be signed in to change notification settings - Fork 16
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 a new endpoint to unreserve a path #1468
Conversation
10e4845
to
90b8275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a minor issue
let(:request_body) { payload.to_json } | ||
|
||
def do_request(body: request_body, headers: {}) | ||
delete request_path, params: body, headers: headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh TIL - did not realise request_path could be in scope here
fail. | ||
- If a path reservation exists for the supplied base_path but a different | ||
publishing_app, the command will fail. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one doing this 👍
spec/commands/unreserve_path_spec.rb
Outdated
|
||
context "when the path is not owned by the app" do | ||
it "returns an error" do | ||
payload = { base_path: "/bar" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks a bit like it's testing what happens when a required parameter is missing rather than owned by a different app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - I was being minimal. Amended :-).
90b8275
to
f0f7207
Compare
What's the motivation for this change? It would be good if you could add that to the commit message. |
This adds a new command to unreserve (destroy) a PathReservation, provided the reservation exists and matches the given publishing app. As we migrate formats from Whitehall to Content Publisher, users sometimes need to revert to Whitehall if certain features aren't available. Because Whitehall assumes it is authoritative for all paths in a format, we need to be able to cleanup the Publishing API in order for the user to re-create their content in Whitehall and re-use a path.
f0f7207
to
8014783
Compare
@cbaines sure, I've updated the commit - the Trello card on this PR should also give more context :-). |
Great, thanks. Scary, but still good to know. |
https://trello.com/c/6s9Bhzyj/632-delete-publishing-api-path-reservations-when-we-discard-drafts
This adds a new command to unreserve (destroy) a PathReservation,
provided the reservation exists and matches the given publishing app.