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: support data loader in frontend #2480

Merged
merged 13 commits into from
Jun 24, 2022

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Jun 20, 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?

Improved OpenAPI import form for a new import experience.

In the current design, he will provide such an import form that can support multiple import data sources with their responsive import options.
image

Related issues

a part of #2465

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

@codecov-commenter
Copy link

Codecov Report

Merging #2480 (09b5350) into master (13670d2) will decrease coverage by 4.04%.
The diff coverage is 26.19%.

@@            Coverage Diff             @@
##           master    #2480      +/-   ##
==========================================
- Coverage   71.55%   67.51%   -4.05%     
==========================================
  Files          60      133      +73     
  Lines        3966     3469     -497     
  Branches        0      846     +846     
==========================================
- Hits         2838     2342     -496     
- Misses        838     1127     +289     
+ Partials      290        0     -290     
Flag Coverage Δ
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 67.51% <26.19%> (?)

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

Impacted Files Coverage Δ
web/src/pages/Route/List.tsx 66.88% <16.66%> (ø)
...b/src/pages/Route/components/DataLoader/Import.tsx 27.27% <27.27%> (ø)
...es/Route/components/DataLoader/loader/OpenAPI3.tsx 33.33% <33.33%> (ø)
api/internal/route.go
api/internal/core/server/server.go
api/internal/handler/migrate/migrate.go
api/internal/handler/stream_route/stream_route.go
api/internal/handler/data_loader/route_export.go
api/main.go
...pi/internal/handler/plugin_config/plugin_config.go
... and 184 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 13670d2...09b5350. Read the comment docs.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 23, 2022

Update

A new import result UI was provided. It is like this, the imported entities total/failed/errors are shown.

image

image

image

@bzp2010 bzp2010 marked this pull request as ready for review June 23, 2022 09:19
'page.route.data_loader.tips.input_task_name': '请输入导入任务名称',
'page.route.data_loader.tips.click_upload': '点击上传',
'page.route.data_loader.tips.openapi3_merge_method':
'是否将 OpenAPI 路径中的多个 HTTP 方法合并为单一路由。当你的路径中多个HTTP方法有不同的细节配置(如securitySchema),你可以关闭这个选项,将为不同的 HTTP 方法生成单独的路由。',

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 23, 2022

About test

Import using the new Data Loader interface. The imported target is the API101 OAS document, which contains three APIs, two of which have two HTTP methods each, thus generating three routes and one upstream in merged method mode and five routes and one upstream in non-merged mode.

For routes a pre-check is performed before writing the data to make sure there are no duplicate routes with the same effect; for upstream, the detection is performed only at the time of writing. Therefore, if the route is duplicated, none of it will be written; if the upstream is duplicated, the route will be written and the upstream will fail to write.

web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved

'page.route.fields.vars.invalid': 'Lütfen gelişmiş eşleşme koşulu yapılandırmasını kontrol edin',
'page.route.fields.vars.in.invalid':
'IN operatörünü kullanırken parametre değerlerini dizi formatında girin.',

'page.route.data_loader.import': 'Import',
Copy link
Member

Choose a reason for hiding this comment

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

why use English here, will it update in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LiteSun

I don't know any Turkish and I can only translate using a translation tool that may not convey the meaning correctly (I had a lot of problems with the English=>Turkish=>English back-and-forth translation) and I think it's better to use English for now and wait for a later translator to do the translation.

BTW, later I will try to import these into crowdin to simplify the internationalization process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @anldrms, Can you help review these translations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Baoyuantop @anldrms If it would help to add a translation of this section, a new PR could be created to do this. ☺️

Copy link
Contributor

@anldrms anldrms Jun 27, 2022

Choose a reason for hiding this comment

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

Sorry for late but I'll glad to help for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
Comment on lines 149 to 150
// remove route
/**/
Copy link
Member

Choose a reason for hiding this comment

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

This section seems to be able to be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

cy.get(selector.drawer).contains('Import Failed').should('be.visible');
cy.get(selector.drawer).contains('Total 5 route imported, 1 failed').click();
cy.get(selector.drawer).contains('is duplicated with route api101_nmm_').should('be.visible');
cy.get(selector.drawer).contains('Close').click();
Copy link
Member

Choose a reason for hiding this comment

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

It is best to add assertions to confirm that the drawer has disappeared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

web/src/pages/Route/components/DataLoader/Import.tsx Outdated Show resolved Hide resolved
@bzp2010 bzp2010 requested review from guoqqqi and LiteSun June 24, 2022 04:27
});
setState('result');
})
.catch(console.error);
Copy link
Member

Choose a reason for hiding this comment

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

should we disable console.error in production environment?

Copy link
Contributor Author

@bzp2010 bzp2010 Jun 24, 2022

Choose a reason for hiding this comment

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

That probably means we don't need to catch and just keep an empty catch function. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't actually need to use it to handle errors either, just to prevent JS errors where exceptions are not caught.

Copy link
Member

Choose a reason for hiding this comment

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

That probably means we don't need to catch and just keep an empty catch function. 🤔

I think console.error can somehow be removed at compile time for the production environment

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can try https://stackoverflow.com/a/41041580 (Not Now)

@bzp2010 bzp2010 merged commit 283b308 into apache:master Jun 24, 2022
hongbinhsu pushed a commit to fitphp/apix-dashboard that referenced this pull request Sep 10, 2022
* upstream/master: (23 commits)
  feat: Add config struct of OpenID-Connect Login (apache#2597)
  feat: set serverUrlMap with env, update cypress, update stylelint (apache#2583)
  chore: fix function name typo (apache#2599)
  fix: page refresh causes deletion exception (apache#2593)
  feat: support show all enable plugin list tab (apache#2585)
  fix: drawer components delete plugin not working (apache#2573)
  feat: add batch delete function for route (apache#2502)
  test: reduce fe ci time (apache#2557)
  doc(csp): add correct csp rule (apache#2548)
  doc: add a notice about the compatibility of Ingress and Dashboard (apache#2552)
  fix: add judgement for last_report_time (apache#2551)
  fix: cli test invalid etcd (apache#2544)
  feat: fix actions version to root version (apache#2521)
  fix: duplicate ID (apache#2501)
  fix: block arbitrary file index (apache#2497)
  docs: update deploy-with-docker.md (apache#2472)
  feat: translating Turkish for new features (apache#2487)
  docs: add new import and export docs to sidebar (apache#2485)
  docs: add data loader and new OpenAPI 3 loader (apache#2484)
  feat: support data loader in frontend (apache#2480)
  ...

# Conflicts:
#	api/internal/route.go
#	web/config/defaultSettings.ts
#	web/yarn.lock
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.

7 participants