Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

(Feature) Show error message when a custom Safe App can't be fetched #2498

Merged
merged 6 commits into from
Jul 7, 2021

Conversation

alongoni
Copy link
Contributor

What it solves

Resolves #1865
Co-authored by @juampibermani

How this PR fixes it

By adding a manifest validation

How to test it

In App section:
1- select "Add custom App"
2- Fill in the App URL field with a random url: E.g.: https://gnosis-safe.io/

Screenshots

image

@alongoni alongoni added the Feature 👑 New functionality label Jun 28, 2021
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jun 28, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

@github-actions
Copy link

const validate = !visited?.appUrl ? undefined : composeValidators(required, validateUrl, uniqueApp(appList))
const validate = !visited?.appUrl
? undefined
: composeValidators(required, validateUrl, validateManifest, uniqueApp(appList))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uniqueApp should probably go before validateManifest.
So that it can check if the URL has been already added before fetching it.

katspaugh
katspaugh previously approved these changes Jun 30, 2021
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! ✅

@alongoni
Copy link
Contributor Author

alongoni commented Jun 30, 2021

We found an error when:
1- Add a valid App URL: https://pr135--safereactapps.review.gnosisdev.com/tx-builder/
2- Delete the App URL
3- Add an invalid App URL: https://pr135--safereactapps.review.gnosisdev.com or https://gnosis.io/
4- Add a valid App URL again: https://pr135--safereactapps.review.gnosisdev.com/tx-builder/
The "Add" button remains disabled.

cc @juampibermani

@github-actions
Copy link

@github-actions
Copy link

@katspaugh katspaugh dismissed their stale review July 6, 2021 08:19

New changes

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@francovenica
Copy link
Contributor

Tried repeated URL, dummy text (not url), a URL that doesn't have a manifest, a URL that has a invalid manifest.

Two of these errors messages are shown in the App name input, not in the URL. According to the snapshot Agustin did the message should be in the URL input, so, is this ok?

snapshots:
image
image
image
image

Are there any more messages?

@francovenica
Copy link
Contributor

Also I tried to reproduce the issue Agustin mentioned, but I wasn't able to.

@dasanra
Copy link
Collaborator

dasanra commented Jul 7, 2021

@francovenica we had to change the approach. Manifest validation is easier on the step where we try to fetch the Safe App information. The form library used has some limitations at the time of asynchronous validations, and it was easier and more simple to solve it like this. Also this way we can show more specific errors, as on the first approach we could only show that "Error loading the app" but with no more feedback

@francovenica
Copy link
Contributor

Given the explanation Dani gave regarding where some error messages are being shown I can pass the ticket.
Haven't found any other error messages.

@dasanra dasanra merged commit ae20684 into dev Jul 7, 2021
@dasanra dasanra deleted the feature/#1865-Load-app-error-message branch July 7, 2021 14:04
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature 👑 New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show error message when a custom Safe App can't be fetched
5 participants