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

escalation policy: add user favorite support #1567

Merged
merged 27 commits into from
Jun 7, 2021
Merged

Conversation

pnengchu
Copy link
Contributor

@pnengchu pnengchu commented May 19, 2021

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
Users may now set favorites on Escalation Policies. This feature is replicated from Schedules and Services.

Which issue(s) this PR fixes:
Fixes #1085

Screenshots:
image

image

Additional Info:
High-Level overview of changes:

  • A migration was added to include a new column, 'tgt_escalation_policy_id' for table 'user_favorites'
  • The structs and schema for both Escalation Policy and Escalation Policy SearchOptions were updated with isFavorite/associated booleans for setting Favorites
  • Relevant SQL queries have been updated to include escalation policies properties
  • Front-end services were addressed to include UI changes when the onClick() is initiated by the end-user.
    -minor refactor of PolicyRouter.js

escalation/policy.go Outdated Show resolved Hide resolved
escalation/search.go Outdated Show resolved Hide resolved
graphql/escalation.go Outdated Show resolved Hide resolved
@@ -7,6 +7,7 @@ const query = gql`
nodes {
id
name
isFavforite
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isFavforite
isFavorite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed. Good catch, thanks!

@pnengchu pnengchu requested a review from dctalbot May 19, 2021 20:26
Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

Looks good, I found one bit that looks missed: when creating a new EP we should set it as a favorite of the current user. See graphql2/graphqlapp/service.go:149 as an example.

Short would be to add a favorite field to CreateEscalationPolicyInput and update the UI to pass it as true

Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

We should write an integration test for this, but I'm OK doing that as a follow up PR. Looks great!

dctalbot
dctalbot previously approved these changes May 20, 2021
Copy link
Contributor

@dctalbot dctalbot left a comment

Choose a reason for hiding this comment

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

Not seeing the escalation policy favorited on creation - double check the input being passed into createPolicy

dctalbot
dctalbot previously approved these changes May 21, 2021
dctalbot
dctalbot previously approved these changes May 21, 2021
Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

We should add the integration test here, otherwise, there's a good chance the follow-up PR doesn't happen until something breaks.

There's already a favorites.ts file with all the other supported ones and a helper function to make it really easy. Let me know if you need help with this, you will just need to add a call to check similar to Schedules, but go to the /services page instead (service create has a SearchSelect for EP).

You will also need to make a small change in support/ep.ts to pass along the favorite parameter, and add the same to ep.d.ts in the types directory. (all under cypress/)

dctalbot
dctalbot previously approved these changes Jun 2, 2021
Comment on lines 57 to 58
? `Unset as a Favorite ${typeName.toLowerCase()}`
: `Set as a Favorite ${typeName.toLowerCase()}`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can use _.startCase so it says Set as Favorite Escalation Policy instead of Set as Favorite escalationpolicy

Suggested change
? `Unset as a Favorite ${typeName.toLowerCase()}`
: `Set as a Favorite ${typeName.toLowerCase()}`
? `Unset as a Favorite ${_.startCase(typeName)}`
: `Set as a Favorite ${_.startCase(typeName)}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed, applied .toLowerCase() to account for Service, Rotation, Schedule checks

web/src/cypress/types/ep.d.ts Show resolved Hide resolved
@@ -35,6 +36,7 @@ function createEP(ep?: EPOptions): Cypress.Chainable<EP> {
name: ep.name || 'SM EP ' + c.word({ length: 8 }),
description: ep.description || c.sentence(),
repeat: ep.repeat || c.integer({ min: 1, max: 5 }),
favorite: ep.favorite,
Copy link
Member

Choose a reason for hiding this comment

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

Fall back to random:

Suggested change
favorite: ep.favorite,
favorite: ep.favorite ?? c.bool(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

dctalbot
dctalbot previously approved these changes Jun 2, 2021
@mastercactapus mastercactapus merged commit fe36cc1 into master Jun 7, 2021
@mastercactapus mastercactapus deleted the esc-policies-fav branch June 7, 2021 15:55
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.

Add Favorites button to Escalation Policies
4 participants