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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Authenticated API datasource creation #6017

Merged
merged 11 commits into from
Jul 22, 2021

Conversation

pranavkanade
Copy link
Contributor

@pranavkanade pranavkanade commented Jul 20, 2021

Description

Added a new card to datasources pane to create REST API datasource, directly from create new tab.

Fixes #5782

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Added a cypress test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Test coverage results 馃И

馃敶 Total coverage has decreased
// Code coverage diff between base branch:release and head branch: feature/oauth-datasource-creation 
Status File % Stmts % Branch % Funcs % Lines
馃敶 total 51.96 (-0.02) 33.55 (-0.02) 30.02 (-0.01) 52.62 (-0.02)
馃敶 app/client/src/pages/Editor/DataSourceEditor/RestAPIDatasourceForm.tsx 25.13 (-0.64) 18.18 (-1.46) 2.94 (0) 28.14 (-0.85)
馃敶 app/client/src/pages/Editor/DataSourceEditor/index.tsx 27.55 (-0.29) 0 (0) 11.11 (0) 28.74 (-0.33)
馃敶 app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx 46.51 (-15.99) 50 (-16.67) 0 (0) 51.28 (-13.24)

@pranavkanade pranavkanade removed the request for review from Nikhil-Nandagopal July 20, 2021 18:52
@pranavkanade
Copy link
Contributor Author

/ok-to-test sha=f902bf5

@pranavkanade
Copy link
Contributor Author

/ok-to-test sha=c06434f

@Nikhil-Nandagopal
Copy link
Contributor

@pranavkanade small change, can we call this authenticated API instead of OAuth

@pranavkanade
Copy link
Contributor Author

/ok-to-test sha=cc6787d

rishabhsaxena
rishabhsaxena previously approved these changes Jul 21, 2021
@pranavkanade pranavkanade changed the title [Feature] Oauth2.0 datasource creation [Feature] Authenticated API datasource creation Jul 21, 2021
rishabhsaxena
rishabhsaxena previously approved these changes Jul 21, 2021
Copy link
Contributor

@rishabhsaxena rishabhsaxena left a comment

Choose a reason for hiding this comment

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

Should we redirect to the view mode of the datasource after saving it, which has a create new query button? We could keep them on the same page as well since there's a new api button here.

Also we should keep the new api button disabled button until we save the datasource, as it still errors out even if the url is filled

@Nikhil-Nandagopal
Copy link
Contributor

Should we redirect to the view mode of the datasource after saving it, which has a create new query button? We could keep them on the same page as well since there's a new api button here.

Also we should keep the new api button disabled button until we save the datasource, as it still errors out even if the url is filled

@rishabhsaxena the new API button from edit mode needs to be removed, it's confusing. Redirecting to the view mode is to give reinforcement that it got saved and the next step is to create a new API

@pranavkanade
Copy link
Contributor Author

@Nikhil-Nandagopal will add saved state for this.

Copy link
Contributor

@rishabhrathod01 rishabhrathod01 left a comment

Choose a reason for hiding this comment

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

When clicking on "create new"
Screenshot 2021-07-21 at 5 47 32 PM

This newly created DataSource using Authenticated API option is visible but not selectable,
Is this expected?

@pranavkanade
Copy link
Contributor Author

@Rishabh-Rathod this is a bug. I can see that on release as well.

@pranavkanade
Copy link
Contributor Author

/ok-to-test sha=72efc2a

rishabhsaxena
rishabhsaxena previously approved these changes Jul 21, 2021
@@ -154,10 +154,14 @@ function NewApiScreen(props: Props) {
}, [plugins]);

const handleCreateAuthApiDatasource = useCallback(() => {
authApiPlugin &&
if (authApiPlugin) {
AnalyticsUtil.logEvent("CREATE_DATA_SOURCE_AUTH_API_CLICK", {
Copy link
Contributor

Choose a reason for hiding this comment

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

_CLICK I guess this suffix would be useful to differentiate b/w frontend / backend events as well

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.

[Feature] Add Auth API Datasource Creation option
4 participants