-
Notifications
You must be signed in to change notification settings - Fork 361
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
M3-5988: Add SMTP restriction support ticket form #8636
M3-5988: Add SMTP restriction support ticket form #8636
Conversation
4be6f19
to
ca92272
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.
packages/manager/src/features/Support/SupportTickets/SupportTicketDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Support/SupportTickets/SupportTicketDrawer.tsx
Outdated
Show resolved
Hide resolved
In addition to what @hana-linode mentioned about the Reproduce:
(This is also reproduced in the |
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 start @mjac0bs I have couple minor suggestions to simplify setting helper text based on ticketType. Other than that over all LGTM!
I agree with @hana-linode feedback on defining outside of the component
packages/manager/src/features/Support/SupportTickets/SupportTicketDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Support/SupportTickets/SupportTicketDrawer.tsx
Outdated
Show resolved
Hide resolved
@hana-linode and @jdamore-linode Latest commit should address issue with inability to submit non-SMTP support tickets - thanks for catching that @jdamore-linode. Can you confirm it's working now? I also added in the entity, so it should display now when an SMTP ticket is submitted as @hana-linode suggested. Still working on the saved description... need to track down why that value isn't being reset when the drawer is closed. |
React.useEffect(() => { | ||
setDescription(formatDescription(smtpFields)); | ||
}, [smtpFields]); | ||
|
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 like this might be the culprit for why smtp fields are displaying in the desc for normal support tickets. If I comment this out and clear my application storage, the desc is no longer filled.
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.
Maybe only format the description on submission and not on every change would be a fix?
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.
Okay, so there is different behavior for the dialog based on how the user cancels it, as documented in this PR:
- If the user hits the Cancel button, the SMTP desc is not visible in the desc of normal support tickets. (Expected behavior).
- If the user hits the 'X', ESC, or clicks outside of the dialog, the contents of the form are saved in local storage rather than cleared. Because the close method didn't know anything about the new SMTP
ticketType
, it was saving that description to local storage and then repopulating it for a normal ticket's description.
Agreed, it would be best to only format the custom fields -> description onSubmit
rather than on each update to the SMTP fields… but since onSubmit
's call to createSupportTicket
needs a description param, we're expecting to use description
from state. If we format and attempt a final state update with setDescription
on submission, the update may not happen before the ticket is created. Always open to suggestions or ideas I've missed, but the latest commit conditionally checking ticketType
in the close
function should address this bug by clearing description
for SMTP tickets.
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.
Disregard the last bit of above ^^ Formatting of the description now only occurs on submission, with an adjustment to the logic that toggles the boolean controlling the submit button from disabled to enabled.
I left the conditional checking ticketType
in close()
in order to always clear the title of the SMTP ticket when the form is closed out. Note: there is still an edge case when a form title can repopulate in a new support ticket's title field if the user was first redirected to support/tickets
with any support ticket having a prefilled title. This behavior exists in prod and predates the SMTP form addition.
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.
Ha! I was just about to post about an issue I was seeing with the ticket contents but I think your latest commit fixed it.
Confirmed that the submit button is fixed and that non-SMTP support tickets can be submitted, confirmed that the SMTP ticket template is no longer pre-populated for non-SMTP tickets, and confirmed that the E2E support test is now passing.
Nicely done @mjac0bs!
@jdamore-linode Yeah, there was an edge case that either needed another conditional checking Thanks for confirming that behavior and E2E is looking good to you! |
Description 📝
What does this PR do?
Adds form fields specific to lifting SMTP restrictions to a support ticket. This is part of efforts to improve the user experience and reduce the amount of back-and-forth between customers and Support when handling requests to remove these restrictions.
Notes:
SupportTicketDrawer
to Formik for improved form state management.Preview 📷
Trigger and fill out the support ticket:
Screen.Recording.2022-12-08.at.2.19.08.PM.mov
Submitted support ticket:
How to test 🧪
What are the steps to reproduce the issue or verify the changes?
How do I run relevant unit or e2e tests?
Units tests:
e2e: