Skip to content
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

Defend from error on invalid URL #6

Merged
merged 12 commits into from
Jun 16, 2021
Merged

Defend from error on invalid URL #6

merged 12 commits into from
Jun 16, 2021

Conversation

fredx30
Copy link
Contributor

@fredx30 fredx30 commented May 11, 2021

Bug Fix - Invalid URLs

This fixes an issue where an invlid URL set on a project produces an error.
This is solved by silencing the error that occurs when the constructor for new URL(...) fails.
Edit:
This is solved by introducing form validation with form locking on invalid input for all the provider forms. Additionally they way URLs get processed has changed see- #6 (comment) -

Example validation failed.
image

Example validation passed-
image

@fredx30 fredx30 added the enhancement New feature or request label May 14, 2021
@Pikabanga
Copy link
Contributor

I'm not sure we should silently swallow this problem. An invalid URL is something that would need to be fixed, and by not showing an error we don't know that there's a problem.

? new URL(this.project.provider.url).hostname
: null;
} catch (e) {
this.providerHostname = null;
Copy link
Contributor

@applejag applejag May 21, 2021

Choose a reason for hiding this comment

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

@Pikabanga brings a fair point. Would rather see a PrimeNG tag with the tooltip of "Invalid provider URL: {error message}" and then setting this.providerHostname = this.project.provider.url directly.

Ex:

NAME GROUP PROVIDER STATUS ACTIONS
sample1 default [error] /some/invalid-url N/A ...
sample2 default github.com N/A ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I wonder if there was a misunderstanding here @fredx30. I meant an [error] tag to the table itself, for when an invalid host name was inserted by other means, such as from an invalid importer path, instead of just hiding the invalid hostname.

@applejag applejag added this to In progress in Backlog via automation May 21, 2021
@fredx30
Copy link
Contributor Author

fredx30 commented Jun 2, 2021

Consider implementing validator (#8 (comment))

@fredx30 fredx30 self-assigned this Jun 11, 2021
@fredx30
Copy link
Contributor Author

fredx30 commented Jun 11, 2021

There is still an error here regarding code duplication. As this is currently executed codeduplication is bordeline unavoidable. I think a solution to the code duplication would be to get started on dynamic forms. And then using them to describe the differences between the 3 providers using a configuration object such as the model to pass in.

@fredx30 fredx30 marked this pull request as ready for review June 11, 2021 10:56
Backlog automation moved this from In progress to Review in progress Jun 11, 2021
@fredx30 fredx30 requested a review from applejag June 14, 2021 10:06
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

LGTM!

Backlog automation moved this from Review in progress to Reviewer approved Jun 15, 2021
@fredx30
Copy link
Contributor Author

fredx30 commented Jun 16, 2021

Resolved conflicts on changelog with rebase.

@fredx30
Copy link
Contributor Author

fredx30 commented Jun 16, 2021

Created an issue (#37) on to deal with the form duplication warned about here- https://app.codacy.com/gh/iver-wharf/wharf-web/pullRequest?prid=7476842&bid=23686990

@fredx30 fredx30 merged commit aff6074 into master Jun 16, 2021
Backlog automation moved this from Reviewer approved to Done Jun 16, 2021
@fredx30 fredx30 deleted the bug/defend-invalid-url branch June 16, 2021 12:23
@applejag applejag mentioned this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants