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

Check that ip should be different from gateway #1669

Merged
merged 8 commits into from
Dec 26, 2023

Conversation

AlaaElattar
Copy link
Contributor

@AlaaElattar AlaaElattar commented Dec 12, 2023

Description

  • Complex error when user add ip same as gateway.
  • Wierd validation also happened.
  • IPv6 can be added to public config of node without Gateway IPv6

Changes

  • Check they're not the same before sending the request.
  • Fix behaviour of validation.
  • Check that both IPv6 and Gateway IPv6 should be added together.

Related Issues

#1618
#1668
#1732

Screencast.from.12-19-2023.01.16.27.PM.webm

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

I think validation should happen before clicking the button and making the request. If 2 IPs are the same, the button should be disabled.

@AlaaElattar AlaaElattar marked this pull request as draft December 12, 2023 15:52
@AlaaElattar
Copy link
Contributor Author

I think validation should happen before clicking the button and making the request. If 2 IPs are the same, the button should be disabled.

image

@AlaaElattar AlaaElattar marked this pull request as ready for review December 13, 2023 10:23
Copy link
Contributor

@mohamedamer453 mohamedamer453 left a comment

Choose a reason for hiding this comment

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

  • The validation isn't very accurate in some cases like the one seen below the validation is triggered with incorrect cases and in the case that should trigger it, it doesn't get triggered.

    Screencast.from.13-12-23.13.50.21.webm

@AlaaElattar AlaaElattar marked this pull request as draft December 14, 2023 12:10
@AlaaElattar AlaaElattar marked this pull request as ready for review December 17, 2023 07:47
@AlaaElattar AlaaElattar marked this pull request as draft December 18, 2023 07:52
@AlaaElattar AlaaElattar marked this pull request as ready for review December 18, 2023 09:55
@0oM4R
Copy link
Contributor

0oM4R commented Dec 18, 2023

please check this comment
#1668 (comment)

@AlaaElattar AlaaElattar marked this pull request as draft December 18, 2023 21:03
@AlaaElattar AlaaElattar marked this pull request as ready for review December 19, 2023 11:33
@AlaaElattar
Copy link
Contributor Author

  • The validation isn't very accurate in some cases like the one seen below the validation is triggered with incorrect cases and in the case that should trigger it, it doesn't get triggered.

    Screencast.from.13-12-23.13.50.21.webm
    

should be fixed

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

All looks good! Just a small enhancement.

packages/playground/src/utils/validators.ts Outdated Show resolved Hide resolved
@zaelgohary zaelgohary merged commit bc29751 into development Dec 26, 2023
3 checks passed
@zaelgohary zaelgohary deleted the development_validation_publicIP branch December 26, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants