ref: migrate externalIssues to scrapsForm#112094
Conversation
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| if (field.required) { | ||
| shape[field.name] = z | ||
| .any() | ||
| .refine(val => val !== null && val !== undefined && val !== '', { |
There was a problem hiding this comment.
should we use .trim() here? AFAIK Django does that by default
There was a problem hiding this comment.
thank you!. shouldn't we check if the message 'this field is required' is displayed?
static/app/components/backendJsonFormAdapter/backendJsonSubmitForm.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const hasFormErrors = useMemo(() => { | ||
| return hasErrorInFields({fields: formFields}); | ||
| return formFields.some(field => field.name === 'error' && field.type === 'blank'); |
There was a problem hiding this comment.
does this need to be inside of a useMemo?
There was a problem hiding this comment.
nope. claude is so eager with those: a39ea1f
| bodyText={ | ||
| <Fragment> | ||
| <Header closeButton> | ||
| <h4>{title}</h4> |
There was a problem hiding this comment.
don't we want to have Heading here?
| <h4>{title}</h4> | |
| <Heading as="h4">{title}</Heading> |
| link="" | ||
| ticketType="" | ||
| instance={{ | ||
| integration: 1 as any, |
There was a problem hiding this comment.
| integration: 1 as any, | |
| integration: '1', |
| // Both values should be visible as selected tags | ||
| expect(screen.getAllByText('bug').length).toBeGreaterThanOrEqual(1); | ||
| expect(screen.getAllByText('feature').length).toBeGreaterThanOrEqual(1); |
There was a problem hiding this comment.
I think this is a bit vague. It's possible that similar words could appear elsewhere on the page, which might lead to false positives. Wouldn't it be better to verify that these values are actually being passed on submit instead?
There was a problem hiding this comment.
we didn’t really make a request, but now we do: 35ef6f1
| await userEvent.click(screen.getByRole('button', {name: 'Create Issue'})); | ||
|
|
||
| // Should NOT have made an API request | ||
| expect(submitRequest).not.toHaveBeenCalled(); | ||
| expect(closeModal).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
shouldn't we check if a user gets a feedback? similar to the test below
There was a problem hiding this comment.
the error toast in the test below is a feature of the mutation, which needs to happen in code we made here. the indicator being shown to the user is a feature of the form elements, which is something we check in the form field tests. but we can check if the fields are invalid: bcdc94b
| // Simulate backend returning HTML error page or non-array response | ||
| MockApiClient.addMockResponse({ | ||
| url: '/search', | ||
| body: '<html>502 Bad Gateway</html>' as any, |
There was a problem hiding this comment.
shouldn't we use the status code here instead, so we can remove the any?
There was a problem hiding this comment.
we need a successful non-json to be returned for this test. but we can remove the assertion: 8ff81cf
|
I just tested the changes with the GitHub integration, and everything worked exactly as expected 🙌 |
leeandher
left a comment
There was a problem hiding this comment.
looks good! wasn't super exhaustive but pulled it locally to review and it seemed totally fine. ran it locally and tested with jira for dynamic form fields, worked great! changes triggered new renders of the fields, previous values persisted, and the final submission matched the ui <3 thank you for working through this!
This PR migrates
externalIssues- both theExternalIssueForand theTicketRuleModalover to the new form system with theBackendJsonFormAdapter.Note that the previous adapter version we had only supported auto-save fields because of settings, so this PR adds “normal” forms:
BackendJsonAutoSaveFormfor auto-saveRendering is “duplicated” on purpose, the fields, schemas and some utils are shared though. On type level, it’s quite hard to create an abstraction that can be shared between auto-save and submit, as auto-save creates one form per field.