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

fix:When create the upstream, some properties can still be edited on … #1828

Merged
merged 7 commits into from
May 3, 2021

Conversation

Engine-AI
Copy link
Contributor

…the preview page bug

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?
#1827

@netlify
Copy link

netlify bot commented Apr 26, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 103c3a3

https://deploy-preview-1828--apisix-dashboard.netlify.app

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #1828 (103c3a3) into master (2f3717f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1828      +/-   ##
==========================================
+ Coverage   72.07%   72.15%   +0.07%     
==========================================
  Files         125      125              
  Lines        2944     2945       +1     
  Branches      711      711              
==========================================
+ Hits         2122     2125       +3     
+ Misses        822      820       -2     
Flag Coverage Δ
frontend-e2e-test 72.15% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
web/src/components/Upstream/UpstreamForm.tsx 81.69% <100.00%> (+0.26%) ⬆️
web/src/helpers.tsx 72.13% <0.00%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f3717f...103c3a3. Read the comment docs.

@@ -83,6 +83,7 @@ const UpstreamForm: React.FC<Props> = forwardRef(

const resetForm = (upstream_id: string) => {
if (upstream_id === undefined) {
setReadonly(disabled);
Copy link
Member

Choose a reason for hiding this comment

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

noted, need @guoqqqi to confirm

Copy link
Member

Choose a reason for hiding this comment

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

Yes, currently all can be edited on the preview page when first configuring upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Does this will affect Route module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this will affect Route module?

Yes!When the router, service, and upstream are created for the first time, this problem also occurs in the upstream involved.

@guoqqqi
Copy link
Member

guoqqqi commented Apr 26, 2021

LGTM!

@liuxiran
Copy link
Contributor

LGTM for code and founction, it would be better to add test case about it @Engine-AI

@Engine-AI
Copy link
Contributor Author

@liuxiran @juzhiyuan ,This is my first time write test case using cypress. Could you please check if my test case complies with the specification

@Engine-AI
Copy link
Contributor Author

@liuxiran @juzhiyuan ,This is my first time write test case using cypress. Could you please check if my test case complies with the specification
This is part of my test case, I think it passed。
image

image

Copy link
Member

@guoqqqi guoqqqi left a comment

Choose a reason for hiding this comment

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

Hi, we can also determine if the input is disabled.

Comment on lines 39 to 46
cy.get(this.domSelector.nodes_0_host).should('be.disabled','true');
cy.get(this.domSelector.nodes_0_port).should('be.disabled','true');
cy.get(this.domSelector.nodes_0_weight).should('be.disabled','true');
cy.get('#timeout_connect').should('be.disabled','true');
cy.get('#timeout_send').should('be.disabled','true');
cy.get('#timeout_read').should('be.disabled','true');
cy.get('#custom_checks_active').should('be.disabled','true');
cy.get('#custom_checks_passive').should('be.disabled','true');
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines 63 to 70
cy.get(this.domSelector.nodes_0_host).should('be.disabled','true');
cy.get(this.domSelector.nodes_0_port).should('be.disabled','true');
cy.get(this.domSelector.nodes_0_weight).should('be.disabled','true');
cy.get('#timeout_connect').should('be.disabled','true');
cy.get('#timeout_send').should('be.disabled','true');
cy.get('#timeout_read').should('be.disabled','true');
cy.get('#custom_checks_active').should('be.disabled','true');
cy.get('#custom_checks_passive').should('be.disabled','true');
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

@guoqqqi If this suggestion or comment is resolved, please click Resolve conversition

Copy link
Member

@guoqqqi guoqqqi left a comment

Choose a reason for hiding this comment

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

Hi~ also😊

@Engine-AI
Copy link
Contributor Author

Hi~ also😊

Thank you @guoqqqi for pointing out my problem

Copy link
Contributor Author

@Engine-AI Engine-AI left a comment

Choose a reason for hiding this comment

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

LGTM!

@juzhiyuan juzhiyuan merged commit 6b997d1 into apache:master May 3, 2021
@Engine-AI Engine-AI deleted the fix-upstream branch May 6, 2021 05:38
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.

6 participants