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

Delete operation in headers policy #928

Merged
merged 7 commits into from
Oct 8, 2018
Merged

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Oct 8, 2018

Closes #922

@davidor davidor requested a review from a team as a code owner October 8, 2018 13:21
@@ -40,7 +44,7 @@
"type": "string"
},
"value": {
"description": "Value that will be added, set or pushed in the header",
"description": "Value that will be added, set or pushed in the header. Not needed when deleting.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution is not ideal. When the operation is "delete", the form will show 2 fields (value and value_type) that will be ignored. However, I didn't find any better alternative that works with https://github.com/mozilla-services/react-jsonschema-form

Dynamic schemas look like a possible solution, we use them in the default credentials policy. However, in this policy, they do not work correctly because they would be defined in an array.

An alternative would be to keep the "operations" array as it is now and define a separate "headers_to_delete" field (array of strings), but I'm not sure that would be much better. For the end-user, having a separate field for the delete operation will not make much sense.

@mayorova , @mikz thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. I think showing extra field is better than adding a whole new array just for deletes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree. As long as it's clear that value is not required for deleting, it's OK.

end)

describe('and the header is already set', function()
it('deletes it', function()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't totally sure about this one but decided to keep it working as it is now.

@davidor davidor merged commit 94d5344 into master Oct 8, 2018
@davidor davidor deleted the headers-policy-delete-op branch October 8, 2018 14:34
@pestanko
Copy link

pestanko commented Oct 9, 2018

@davidor This one is not in the DR1 right?

@mikz
Copy link
Contributor

mikz commented Oct 9, 2018

@pestanko nope, 3scale-2.4.0-DR1...master

@pestanko
Copy link

pestanko commented Oct 9, 2018

@mikz I have tested this on nightly, so far this works.

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.

4 participants