console: fix visiting view modify page overwriting raw sql content #4810

soorajshankar · 2020-05-18T21:11:49Z

Description

Preserve data in local storage to ensure the user-provided data in raw SQL editor. data will preserve even after going to view modify tab.

Changelog

CHANGELOG.md is updated with user-facing content relevant to this PR.

Affected components

Related Issues

close #4798

Solution and Design

Since data/sql is used commonly a URL query string has been implemented. this helps to identify the container is mounted with modify mode or run sql mode.
using this identity, local storage value/redux value is used in the editor.

Steps to test and verify

Go to Run SQL page and type some text.
go to any view under tables section.
click on modify tab.
go back run SQL page

Current Behaviour

Run SQL page shows the data from the lastly viewed view under table.

Expected Behaviour

Run SQL page should show the user entered data from the text editor.

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No
Yes
- Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

No
Yes
- Does run_sql auto manages the new metadata through schema diffing?
  - Yes
  - Not required
- Does run_sql auto manages the definitions of metadata on renaming?
  - Yes
  - Not required
- Does export_metadata/replace_metadata supports the new metadata added?
  - Yes
  - Not required

GraphQL

No new GraphQL schema is generated
New GraphQL schema is being generated:
- New types and typenames are correlated

Breaking changes

hasura-bot · 2020-05-18T21:25:57Z

Review app for commit 38cfb44 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-38cfb448

CHANGELOG.md

Co-authored-by: Tirumarai Selvan <tirumarai.selvan@gmail.com>

tirumaraiselvan

changelog

hasura-bot · 2020-05-19T07:30:28Z

Review app for commit 9651787 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-9651787b

rikinsk

@soorajshankar as we discussed, lets make modify of view happen in place and decouple it completely from the RawSQL page. We can allow the user to edit the view definition in the modify page itself and if we detect a change a Save button can be enabled.

Actually lets make the behaviour similar to the way we handle custom sql functions. The modify page willl have a code-block with the view definition and modify button which redirects to RawSQL page with the view definition prefilled

netlify · 2020-05-19T09:44:43Z

Deploy preview for hasura-docs ready!

Built with commit 3c4956e

https://deploy-preview-4810--hasura-docs.netlify.app

soorajshankar · 2020-05-19T09:55:48Z

@rikinsk
Previous Behaviour

This PR Behaviour

hasura-bot · 2020-05-19T09:56:46Z

Review app for commit 43a1d38 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-43a1d388

rikinsk

UX approved

hasura-bot · 2020-05-19T10:18:52Z

Review app for commit 467732e deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-467732e6

beerose

Left some comments.

console/src/components/Services/Data/RawSQL/molecules/NotesSection.js

console/src/components/Services/Data/RawSQL/RawSQL.js

> Alert > NotesSection

hasura-bot · 2020-05-20T09:20:28Z

Review app for commit 99c4a27 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-99c4a271

beerose

Instead of adding new state variable, we should implement the same solution as is now for modifying custom functions and work on how data is handled in Modify Actions. Right now when a view table is loaded, fetchViewDefinition function fetches view definition and stores it in RawSql/raw_sql, and this part should be decoupled. Raw SQL state shouldn't be updated on actions in the Modify view as they are two separate parts of the console.

sql: state.rawSQL.viewSql,

This sql value should be taken from the Modify state, thus fetchViewDefinition should dispatch an action that updates Modify state. Then we could use this as previously:

<ViewDefinitions modifyViewOnClick={modifyViewOnClick} sql={sql} />

And set raw_sql only if the user clicks modfiy and is redirected to the Raw SQL page.
For that we could actually use RawSqlButton component that is used for custom functions: https://github.com/hasura/graphql-engine/blob/master/console/src/components/Services/Data/TableModify/ComputedFieldsEditor.js#L305.

Btw, from the user perspective, let's say I have a query that I run very often. As long as it's persisted in local storage, everything is cool. But if I edit a view definition, it's quite a pity that my original query is overwritten in local storage. The same happens if I edit or create a custom function.

I see three possible solutions to this problem:

Don't store SQL string in local storage when it's derived from view definition or custom function.
Decouple modifying custom functions and views from the Raw SQL page and inline it on Modify pages.
(My favorite) Create a history of executed queries in the Raw SQL page in a similar way as it is in the GraphiQL section (with a clear all button). Then we wouldn't need to worry about possible overrides and it would also be an extremely useful feature.

Thoughts @rikinsk?

hasura-bot · 2020-05-26T08:08:51Z

Review app for commit ae6de04 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-ae6de043

beerose · 2020-05-26T10:18:10Z

@soorajshankar Can you take a look at the failing test?

hasura-bot · 2020-05-26T15:54:26Z

Review app for commit b6298cf deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-b6298cf2

hasura-bot · 2020-05-26T17:57:15Z

Review app for commit 12a1968 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-12a19689

rikinsk · 2020-05-27T08:33:01Z

@beerose

Don't store SQL string in local storage when it's derived from view definition or custom function.

This might be just as confusing as I would lose any edits I make to the edit view SQL on the Raw SQL page if I navigate. Typically I just expect the last content on the RawSQl page to persist so this doesnt seem ideal.

Decouple modifying custom functions and views from the Raw SQL page and inline it on Modify pages.

This would be the best solution but is a little tricky because we let the user execute any SQL and that can allow them to do unexpected things. e.g. if they drop the current view, the current page we are on will become a 404. If we can list all the possible end states and handle them or ensure that the user can only modify the current view (make the "create or replace view as " part of the statement fixed) this can be a good option.

(My favorite) Create a history of executed queries in the Raw SQL page in a similar way as it is in the GraphiQL section (with a clear all button). Then we wouldn't need to worry about possible overrides and it would also be an extremely useful feature.

This seems generally useful even beyond this issue and surprising no one has asked for this yet. Maybe we can create an issue and see the interest it gets and then decide when to pick it up.

beerose · 2020-05-27T08:40:50Z

@rikinsk I propose to keep the scope of this PR to only fix #4798, and for those two ideas (decoupling edit and query history) create issues and decide whether to do them or not later depending on the users' interest.

rikinsk · 2020-05-27T08:53:30Z

@rikinsk I propose to keep the scope of this PR to only fix #4798, and for those two ideas (decoupling edit and query history) create issues and decide whether to do them or not later depending on the users' interest.

Definitely. I think the current UX is good to go.

console/cypress/integration/data/raw-sql/spec.js

console/src/components/Services/Data/RawSQL/RawSQL.js

hasura-bot · 2020-05-27T10:15:43Z

Review app for commit a6aad5c deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-a6aad5c3

console/src/components/Services/Data/RawSQL/molecules/NotesSection.tsx

…o fix/raw-sql-persist-data-#4798

hasura-bot · 2020-05-29T06:01:57Z

Review app for commit 3c4956e deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-3c4956e6

console/src/components/Services/Data/RawSQL/RawSQL.js

hasura-bot · 2020-05-29T07:18:14Z

Review app for commit 3879e81 deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-3879e81c

hasura-bot · 2020-05-29T07:45:39Z

Review app for commit c0cc2fe deployed to Heroku: https://hge-ci-pull-4810.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4810-c0cc2fe6

hasura-bot · 2020-05-29T08:41:05Z

Review app https://hge-ci-pull-4810.herokuapp.com is deleted

…lose hasura#4798) (hasura#4810)

soorajshankar added 2 commits May 19, 2020 02:29

Raw SQL preserve editor values hasura#4798

d851a27

changelog

38cfb44

soorajshankar requested review from a team as code owners May 18, 2020 21:11

soorajshankar assigned beerose and wawhal May 18, 2020

wawhal assigned rikinsk and unassigned beerose and wawhal May 19, 2020

tirumaraiselvan reviewed May 19, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

9651787

Co-authored-by: Tirumarai Selvan <tirumarai.selvan@gmail.com>

tirumaraiselvan approved these changes May 19, 2020

View reviewed changes

rikinsk requested changes May 19, 2020

View reviewed changes

rikinsk added the c/console Related to console label May 19, 2020

replaced ace editor with code block> view

43a1d38

Merge branch 'master' into fix/raw-sql-persist-data-#4798

467732e

rikinsk assigned beerose and unassigned rikinsk May 19, 2020

rikinsk requested a review from beerose May 19, 2020 10:04

rikinsk approved these changes May 19, 2020

View reviewed changes

beerose suggested changes May 19, 2020

View reviewed changes

TS migration

99c4a27

> Alert > NotesSection

jsdoc and Alert moved to common components

98309b5

beerose suggested changes May 25, 2020

View reviewed changes

Moved viewDefSQL to tables/modify in redux

ae6de04

cypress tests > match with editor debounce time

b6298cf

optimised raw sql tests

12a1968

beerose suggested changes May 27, 2020

View reviewed changes

console/cypress/integration/data/raw-sql/spec.js Show resolved Hide resolved

console/src/components/Services/Data/RawSQL/RawSQL.js Show resolved Hide resolved

soorajshankar added 2 commits May 27, 2020 15:26

sync with master

7db732e

reverted preserve rawSQL localstoreage logic

a6aad5c

beerose reviewed May 28, 2020

View reviewed changes

console/src/components/Services/Data/RawSQL/molecules/NotesSection.tsx Outdated Show resolved Hide resolved

soorajshankar added 2 commits May 29, 2020 11:15

review comments hasura#4810

6f298c9

Merge branch 'master' of https://github.com/hasura/graphql-engine int…

3c4956e

…o fix/raw-sql-persist-data-#4798

Merge branch 'master' into fix/raw-sql-persist-data-#4798

3879e81

beerose reviewed May 29, 2020

View reviewed changes

console/src/components/Services/Data/RawSQL/RawSQL.js Outdated Show resolved Hide resolved

review comments hasura#4810

c0cc2fe

beerose self-requested a review May 29, 2020 07:36

beerose approved these changes May 29, 2020

View reviewed changes

rikinsk changed the title ~~Raw SQL preserve editor values #4798~~ console: fix visiting view modify page overwriting raw sql content May 29, 2020

rikinsk merged commit b2fd57a into hasura:master May 29, 2020

stevefan1999-personal pushed a commit to stevefan1999-personal/graphql-engine that referenced this pull request Sep 12, 2020

console: fix visiting view modify page overwriting raw sql content (c…

9fe7180

…lose hasura#4798) (hasura#4810)

console: fix visiting view modify page overwriting raw sql content #4810

console: fix visiting view modify page overwriting raw sql content #4810

Uh oh!

Conversation

soorajshankar commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog

Affected components

Related Issues

Solution and Design

Steps to test and verify

Current Behaviour

Expected Behaviour

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

Uh oh!

hasura-bot commented May 18, 2020

Uh oh!

Uh oh!

tirumaraiselvan left a comment

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented May 19, 2020

Uh oh!

rikinsk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soorajshankar commented May 19, 2020

Uh oh!

hasura-bot commented May 19, 2020

Uh oh!

rikinsk left a comment

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented May 19, 2020

Uh oh!

beerose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasura-bot commented May 20, 2020

Uh oh!

beerose left a comment

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented May 26, 2020

Uh oh!

beerose commented May 26, 2020

Uh oh!

hasura-bot commented May 26, 2020

Uh oh!

hasura-bot commented May 26, 2020

Uh oh!

rikinsk commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beerose commented May 27, 2020

Uh oh!

rikinsk commented May 27, 2020

Uh oh!

Uh oh!

Uh oh!

hasura-bot commented May 27, 2020

Uh oh!

Uh oh!

hasura-bot commented May 29, 2020

Uh oh!

soorajshankar commented May 18, 2020 •

edited

Loading

rikinsk left a comment •

edited

Loading

netlify bot commented May 19, 2020 •

edited

Loading

rikinsk commented May 27, 2020 •

edited

Loading