-
Notifications
You must be signed in to change notification settings - Fork 111
update support ticket to support categories, project, and target options #6689
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
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/services/api/src/modules/support/resolvers/SupportTicket.ts
Outdated
Show resolved
Hide resolved
@@ -30,6 +30,9 @@ export default gql` | |||
|
|||
input SupportTicketCreateInput { | |||
organizationSlug: String! | |||
project: String | |||
target: String | |||
category: SupportCategory! |
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.
Minor: I defer to my colleagues, but I think we should default this or make it nullable to avoid taking any downtime during the deployment window.
We don't get a ton of support tickets so I doubt it'd cause an issue, but I'm extra cautious.
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.
Great work on this overall. These are pretty minor details that I'd prefer be slightly different.
@@ -568,6 +578,12 @@ export class SupportManager { | |||
}); | |||
const customerType = this.resolveCustomerType(organization); | |||
|
|||
const formattedBody = ` "Category: " + ${request.data.category ? request.data.category : null}\n\n | |||
"Project: " + ${request.data.project ? request.data.project : null}\n\n | |||
"Target: " + ${request.data.target ? request.data.target : null}\n\n |
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.
minor: since this is just text and not jsx, I'd prefer "not selected" as the default over null
@@ -606,6 +622,9 @@ export class SupportManager { | |||
metadata: { | |||
ticketDescription: input.description, | |||
ticketPriority: input.priority, | |||
ticketCategory: input.category ?? '', | |||
ticketProject: input.project ?? '', | |||
ticketTarget: input.target ?? '', |
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.
prefer not including the metadata if it isnt present here.
I.e. ...(input.category ? { ticketCategory: input.category } : {}),
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.
I added a few comments
@@ -95,6 +101,13 @@ export default gql` | |||
fromSupport: Boolean! | |||
} | |||
|
|||
enum SupportCategory { |
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.
Please use SupportCategoryType
instead of SupportCategory
in order to follow our guideline for new GraphQL schema additions.
project: String | ||
target: String |
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.
Why not reference the Project
and Target
types here instead of String
?
project: String | ||
target: String |
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.
Why not use the project id and target id as the input here instead of the slug?
It would be a more stable reference compared to just the slug.
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.
Is it not easier to read the ticket if we use the names?
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.
Somewhat recently we added support for UUIDs in addition to paths (combination of slugs).
The reason for this change was because these UUIDs don't change if people rename things.
For a support ticket, we typically get to them quick enough that I dont think it would make a huge difference, but on the off chance that they change the name of a resource, a UUID would be better because then we don't need to ask again.
Readability isn't a concern -- it's all about being able to look up information in logs etc.
category: SupportCategory! | ||
project: String | ||
target: String |
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.
Do these fields actually work? They are not used on the frontend.
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.
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.
They are sent in the input but they may not be getting resolved.
For simplicty, I think these fields should be removed from the SupportTicket type. The requirement was to add this information to the support ticket body. I dont think we need it both in the body and in other fields.
Background
Updated the support ticket to include categories as well as optionally allow selection of the project and target.
#6622
Description
Update the TargetSelector and ProjectSelector
Update the Ticket page to allow for selection of the Target, Project, and Category
Update the mutation for new tickets to support the category, target, and project changes.
network sends the new inputs but I can't test further