Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Whatever we're doing to make nicer error messages, we might want to also apply to tag creation if that's easy. This is from the "new tag" button on the tags page: 
Also, I don't know if this is something about a dev environment or something else, but for one of the two errors from the rename path, I got an error message with a timestamp in it: 
The other one was as shown in the description. |
Oaphi
changed the title
| if(isFailed) { | ||
| const { errors = [], message } = data; | ||
|
|
||
| if (message) { | ||
| const fullMessage = | ||
| errors.length > 1 | ||
| ? `${message}:<ul>${errors.map((e) => `<li>${e.trim()}</li>`).join('')}</ul>` | ||
| : errors.length === 1 | ||
| ? `${message} (${errors[0].toLowerCase().trim()})` | ||
| : message; | ||
|
|
||
| QPixel.createNotification('danger', fullMessage); | ||
| } | ||
| else { | ||
| for (const error of errors) { | ||
| QPixel.createNotification('danger', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling failed responses is as follows:
- If only the
messagefield is present, show it directly; - If only the
errorsfield is present, create a notification for each error listed; - If both
messageanderrorsare present:- if only one error is present, append a lowercase version of it in parentheses (
<message> (<error>)); - otherwise, use
messageas the notification's header and displayerrorsas an unordered list (matches what ourflashlogic does);
- if only one error is present, append a lowercase version of it in parentheses (
@cellio IIRC, the timestamp is shown when there's already an error notification present on the page - see |
Ah, ok! If this PR didn't add it, then definitely don't do anything about it here. And maybe not at all; I didn't realize the connection to the other error that was already on the page, and in that rare case it does seem useful to distinguish somehow. Let's leave it alone. |
cellio
left a comment
cellio
left a comment
There was a problem hiding this comment.
Tested with latest changes. LGTM. Didn't code-review JS.
3 participants
closes #1851
Also fixes an old bug preventing validation for tags with spaces from taking effect.
Error notification when the new tag name contains spaces:
Error notification when the new tag name matches an already existing tag:
As a treat, it also ensures old tag names are quoted in the rename prompt (on a tangent, we should move from a prompt to a proper modal dialog, but this is outside of the scope of this PR):
Compared to how it is now:
Finally, it also changes error handling for tag creation (see #1852 (comment)) a bit:
removes duplicate error flashes after failing to create tags;
improves the combined error message shown upon failure to create a tag (no actual changes, just a nice side-effect of other changes):