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: advance matching in the route create page causes the page to crash #2440

Merged
merged 8 commits into from
May 30, 2022
Merged

fix: advance matching in the route create page causes the page to crash #2440

merged 8 commits into from
May 30, 2022

Conversation

Si-ege
Copy link
Contributor

@Si-ege Si-ege commented Apr 24, 2022

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

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

What changes will this PR take into?

Please update this section with detailed description.

Related issues

fix/resolve #2438
1.fix matching in the route create page causes the page to crash

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@Si-ege
Copy link
Contributor Author

Si-ege commented Apr 24, 2022

My understanding of this issue is:

  1. the method toString is not executed when the default value of the reverse field is undefined

@Si-ege Si-ege closed this Apr 24, 2022
@Si-ege Si-ege reopened this Apr 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #2440 (1947e24) into master (0e6411f) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2440      +/-   ##
==========================================
+ Coverage   68.57%   68.80%   +0.23%     
==========================================
  Files         131      131              
  Lines        3427     3427              
  Branches      831      833       +2     
==========================================
+ Hits         2350     2358       +8     
+ Misses       1077     1069       -8     
Flag Coverage Δ
frontend-e2e-test 68.80% <100.00%> (+0.23%) ⬆️

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

Impacted Files Coverage Δ
...pages/Route/components/Step1/MatchingRulesView.tsx 79.16% <100.00%> (+4.16%) ⬆️
web/src/pages/Route/transform.ts 80.42% <0.00%> (+2.11%) ⬆️

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 0e6411f...1947e24. Read the comment docs.

@Baoyuantop
Copy link
Contributor

Hi @Si-ege, we need E2E test for this change:

  1. Go to Route List and click Create
  2. Fill in the necessary information and click the Add button in the Advanced Routing Matching Conditions section.
  3. Fill in everything in the Create Rule except Reverse the result(!) field
  4. Click Confirm and we can see the data in the table and Reverse the result(!) field is false.
  5. We can create this route successfully

@Si-ege
Copy link
Contributor Author

Si-ege commented Apr 25, 2022

ok

@Baoyuantop
Copy link
Contributor

Hi @Si-ege, thank you for your contribution. For the E2E test section, we would prefer to create a new test case, meaning to write the test in a new it() instead of changing the previous test.

cy.get(selector.value).type(data.value);
cy.contains('Confirm').click();
cy.get(selector.rowcard).should('be.visible');
cy.get(selector.rowcard).get('tr>td').eq(2).should('have.value', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it show up as false here?

@@ -148,7 +148,8 @@ const MatchingRulesView: React.FC<RouteModule.Step1PassProps> = ({
{
title: formatMessage({ id: 'page.route.reverse' }),
key: 'reverse',
render: (text: RouteModule.MatchingRule) => text.reverse.toString(),
render: (text: RouteModule.MatchingRule) =>
text.reverse === undefined ? text.reverse : text.reverse.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is more appropriate to set an initial value for reverse in the Form.

@guoqqqi
Copy link
Member

guoqqqi commented May 6, 2022

ping @Si-ege any update?

@Baoyuantop Baoyuantop merged commit 5e624a5 into apache:master May 30, 2022
hongbinhsu pushed a commit to fitphp/apix-dashboard that referenced this pull request Jun 16, 2022
* upstream/master:
  test: remove stale E2E cases (apache#2475)
  feat: basic support Apache APISIX 2.14.1 (apache#2464)
  fix: Users can create a Consumer in Dashboard without enabling the plugin (apache#2442)
  fix: change "Route List" to "Routes" (apache#2453)
  fix: advance matching in the route create page causes the page to crash (apache#2440)
  feat: release 2.13 (apache#2458)
  feat: support Turkish Language (apache#2452)

# Conflicts:
#	web/package.json
bzp2010 pushed a commit to bzp2010/apisix-dashboard that referenced this pull request Oct 31, 2022
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.

bug: advance matching in the route create page causes the page to crash
6 participants