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

Adding escalation level to ManageIncidentOptions #366

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

sim1s
Copy link
Contributor

@sim1s sim1s commented Sep 27, 2021

No description provided.

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I have one observation / question about the tests, but beyond that this looks good.

@@ -210,6 +210,42 @@ func TestIncident_Manage_assignments(t *testing.T) {
testEqual(t, want, res)
}

func TestIncident_Manage_esclation_level(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are we actually testing here? It doesn't seem like we do anything to confirm that the escalation_level field is passed in the JSON in the request body.

Would it make sense to update the HTTP handler to parse the JSON request body in this test, and fail if the escalation_level JSON key is either missing or not the expected value?

@theckman theckman added this to the v1.5.0 milestone Sep 28, 2021
@theckman
Copy link
Collaborator

theckman commented Nov 9, 2021

@sim1s wanted to see if you'd have some time to take a look at the question I had in the requested changes.

@theckman theckman linked an issue Nov 9, 2021 that may be closed by this pull request
Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

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

I'll address my comments with the tests at a later time. This is true not only of these tests but others.

@theckman theckman merged commit 2b775e1 into PagerDuty:master Nov 9, 2021
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.

ManageIncidentsOptions doesn't take EscalationLevel
2 participants