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

feat: improve the config plugin button enable disable status #1610

Merged
merged 18 commits into from
Apr 18, 2021
Merged

feat: improve the config plugin button enable disable status #1610

merged 18 commits into from
Apr 18, 2021

Conversation

stu01509
Copy link
Contributor

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.

Improve the config plugin button status, also I didn't found the 启用 and 禁用 in our current locales files, please help me to translate it, thanks.
image

@juzhiyuan juzhiyuan requested a review from LiteSun March 18, 2021 02:09
@juzhiyuan
Copy link
Member

Hi @stu01509, you could cc @guoqqqi to guide you how to resolve CI testcases.

@guoqqqi
Copy link
Member

guoqqqi commented Mar 20, 2021

The test failed because when the button was clicked, the button became Disable and the Disable button was not found.
You can fix line 59 by changing
cy.contains('Enable') to cy.get('button') to fix it.

image

@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #1610 (e9a8d9f) into master (c68e2d2) will decrease coverage by 4.98%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1610      +/-   ##
==========================================
- Coverage   72.01%   67.02%   -4.99%     
==========================================
  Files         160      160              
  Lines        5820     5822       +2     
  Branches      651      653       +2     
==========================================
- Hits         4191     3902     -289     
- Misses       1385     1607     +222     
- Partials      244      313      +69     
Flag Coverage Δ
backend-e2e-test 62.30% <ø> (ø)
backend-e2e-test-ginkgo 49.16% <ø> (+0.09%) ⬆️
backend-unit-test ?
frontend-e2e-test 72.49% <100.00%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
web/src/components/Plugin/PluginPage.tsx 98.71% <100.00%> (+0.03%) ⬆️
api/internal/utils/runtime/runtime.go 0.00% <0.00%> (-64.29%) ⬇️
api/internal/core/store/validate_mock.go 0.00% <0.00%> (-50.00%) ⬇️
api/internal/filter/authentication.go 47.22% <0.00%> (-30.56%) ⬇️
api/internal/handler/service/service.go 62.60% <0.00%> (-29.57%) ⬇️
api/internal/core/store/store.go 58.43% <0.00%> (-29.52%) ⬇️
api/internal/filter/ip_filter.go 48.71% <0.00%> (-23.08%) ⬇️
api/internal/handler/global_rule/global_rule.go 64.51% <0.00%> (-19.36%) ⬇️
...pi/internal/handler/plugin_config/plugin_config.go 59.57% <0.00%> (-18.09%) ⬇️
api/internal/utils/json_patch.go 44.82% <0.00%> (-13.80%) ⬇️
... and 14 more

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 c68e2d2...e9a8d9f. Read the comment docs.

@stu01509
Copy link
Contributor Author

Hi @juzhiyuan and @guoqqqi

Update the commit :)

@nic-chen
Copy link
Member

@stu01509 please sync codes from branch master

@stu01509
Copy link
Contributor Author

@stu01509 please sync codes from branch master

Done.

@juzhiyuan
Copy link
Member

@stu01509 Hi, here are some conflicts, please take a look.

@stu01509
Copy link
Contributor Author

stu01509 commented Apr 2, 2021

Hi @juzhiyuan

Update the commit :)

@juzhiyuan
Copy link
Member

@stu01509 Hi, please run yarn build, and you will get error messages.

@stu01509
Copy link
Contributor Author

stu01509 commented Apr 2, 2021

Hi @juzhiyuan and @guoqqqi

Update the commit, and passed the CI,
please review it again :)

@guoqqqi
Copy link
Member

guoqqqi commented Apr 5, 2021

LGTM, Thanks!

juzhiyuan
juzhiyuan previously approved these changes Apr 6, 2021
@juzhiyuan juzhiyuan mentioned this pull request Apr 12, 2021
11 tasks
@netlify
Copy link

netlify bot commented Apr 12, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 5bc61a4

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

@juzhiyuan
Copy link
Member

ping @liuxiran @bzp2010

@juzhiyuan
Copy link
Member

@stu01509 Hi, to prevent from CI failed, please merge the latest codes into your branch :) Thanks

@juzhiyuan
Copy link
Member

image

I noticed this CI failed, that's because this PR changes something related to testcases, it's not hard to fix it.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@f1b8f0c). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1610   +/-   ##
=========================================
  Coverage          ?   72.14%           
=========================================
  Files             ?      125           
  Lines             ?     2908           
  Branches          ?      701           
=========================================
  Hits              ?     2098           
  Misses            ?      810           
  Partials          ?        0           
Flag Coverage Δ
frontend-e2e-test 72.14% <100.00%> (?)

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

Impacted Files Coverage Δ
web/src/components/Plugin/PluginPage.tsx 98.70% <100.00%> (ø)

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 f1b8f0c...5bc61a4. Read the comment docs.

@stu01509 stu01509 requested a review from juzhiyuan April 18, 2021 04:43
@stu01509
Copy link
Contributor Author

Hi @juzhiyuan

Update the commit, sorry for the delayed reply.

@@ -57,7 +57,7 @@ context('Create and delete route with limit-count form', () => {

// config limit-count form with local policy
cy.contains(this.domSelector.pluginCard, 'limit-count').within(() => {
cy.contains('Enable').click({
cy.get('button').click({
Copy link
Member

Choose a reason for hiding this comment

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

If we have multiple buttons, which one it will take?

@juzhiyuan juzhiyuan dismissed their stale review April 18, 2021 09:35

Still need some tests :)

@juzhiyuan
Copy link
Member

@guoqqqi Please recheck this PR, and merge the latest master codes to this one, and see if it's working properly, thanks!

@juzhiyuan juzhiyuan merged commit c9d2bd3 into apache:master Apr 18, 2021
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.

Config Plugin button enable status
7 participants