-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix: ds-839 block modal form submissions #517
Conversation
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 have tested that locally. Covered create credential modal, create source modal, edit source modal and new scan modal. Edit credential is worked on in another PR, we will need to test that modal once at least one of PRs is merged and other is rebased.
Overall it works as we intended for this stage. There are three problems I have noticed, only first of which I would consider required. I'm putting all three here in case they can be fixed cheaply while doing the required one. If not, let me know, I will create tickets for them and we will handle them in due time.
Problem 1 (required)
If I don't fill scan name, there are going to be two toast notifications.
Steps:
- Create credential and scan of any type
- Go to Sources and click "Scan" link for any source
- Leave "Name" field empty
- Click Save
Result:
Problem 2 (optional)
Error messages from server are processed before being displayed. As a result there are HTML entity codes and backslash character may be missing.
Steps:
- Create credential of network type
- Go to Sources and click "Add Source" -> "Network range"
- Fill all fields
- In "Search addresses", insert
192.1.5.1/
Result:
(note #x2F;
and that there is single backslash instead of two)
Problem 3 (optional)
Field names from server errors are missing. Error messages are constructed by extractErrorMessage
, but that one looks only into response values - while response object keys contain important contextual information.
At one point we have discussed introducing a copy of extractErrorMessage
function, but one that would also display error keys. The idea would be that we don't want to modify existing function due to regression testing surface that would require. But not sure if it's something we can do, as I don't see extractErrorMessage
called explicitly in this PR anywhere.
Steps:
- Go to Credentials and click "Add Credential" -> "Network range"
- Leave form empty and click "Save"
Result (no duplicated "This field is required.")
@mirekdlugosz not going to be expanding this PR reasons
Iteration to maintain timeline is the key here, expanding this PR instead would be scope creep * edit: my internal volunteer time still needs to be considered. you all are more than welcome to iterate on this work in subsequent PRs |
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.
we've agreed that we will iterate on this one.
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.
@justinkreft and @gtanzillo agree problem number 1 is not a release blocker. I have created a Jira DISCOVERY-859 so we can fix that later.
For problem number 2, I have created Jira DISCOVERY-856.
For problem number 3, I had hoped this is something we can improve cheaply before the release. Since we can't, I feel that after the release our efforts are better spent on putting these error messages near fields instead of toasts. Something like a work done in #511 . So there is no separate ticket for this.
What's included
Notes
Promise.reject
when readyHow to test
Local run check
$ npm install
$ npm run start:stage
Example
expected basic toast notifications
Updates issue/story
DISCOVERY-839