-
Notifications
You must be signed in to change notification settings - Fork 2.8k
console: allow configuring session_argument for custom functions (close #4499) #4922
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
console: allow configuring session_argument for custom functions (close #4499) #4922
Conversation
|
Deploy preview for hasura-docs ready! Built with commit 18fca1a |
|
Review app for commit 5cb419c deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit 054c31a deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit 33b3343 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very well written code! Nice!
A couple of points regarding functionality:
-
After clicking
Savefor setting session argument app redirects todata/schema/public. Is it a bug or a feature? -
Can we prefill input after setting the session variable? Right now it remains empty after visiting the modify page again.
-
The name of the migration should indicate setting session variable.
-
This is generated down migration — check missing name:
- args:
name: ""
schema: public
type: untrack_function
- args:
name: get_session_role
schema: public
type: track_function
Is this down migration needed?
- Let's be consistent with the naming. Sometimes there's
Session variable argument, sometimesSession argument variable. The same goes for the casing.
Example:
In the docs, we refer to this by session variables, and I think we should stick to just session variable, lowercase.
- There's no error notification on server error. Can you check this?
- What about having
Learn morelink to the docs for it? cc @rikinsk
console/src/components/Services/Data/Function/Modify/SessionVarSection.tsx
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/Modify/SessionVarSection.tsx
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/Modify/ModifyCustomFunction.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/customFunctionReducer.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/customFunctionReducer.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/customFunctionReducer.js
Outdated
Show resolved
Hide resolved
console/src/components/Services/Data/Function/customFunctionReducer.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few points from a UX perspective:
- Can we make this page look consistent with Modify Page for views. i.e. Add a heading called
Function definitionand move theModifybutton next to it. - Use the term
Session argumenteverywhere (argument doesnt need to start with capital). - Update tooltip text to:
the function argument into which hasura session variables will be passed - Add a
Know morelink next to the tooltip linking tohttps://hasura.io/docs/1.0/graphql/manual/schema/custom-functions.html#accessing-hasura-session-variables-in-custom-functions(there is aKnowMoreLinkcomponent for this) - in the placeholder change
hasura-sessiontohasura_session.-are not allowed in SQL - After saving the session argument, just close the session argument editor. No redirect to schema page.
- Once a session argument is added, prefill its value in the input box when edit is clicked
- Disable the save button when the input value is unchanged from the current value
- Currently, there is no way to remove a session argument once set. Allow users to save an empty string as well which we can interpret as no session argument is set
…o feature/session_argument-as-sql-function-argument-#4499
…' of https://github.com/soorajshankar/graphql-engine into feature/session_argument-as-sql-function-argument-#4499
|
Review app for commit 8bde531 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit 63a2179 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit 2bb4a5a deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit fe37b34 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit f2d97e7 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
…o feature/session_argument-as-sql-function-argument-#4499
|
Review app for commit da9260f deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
rikinsk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX approved
beerose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Down migration for setting session arg is incorrect:
- args:
name: customfunctionwithsessionarg
schema: public
type: untrack_function
- args:
configuration: {}
function:
name: customfunctionwithsessionarg
schema: public
type: track_function
Missing version: 2.
…o feature/session_argument-as-sql-function-argument-#4499
|
Review app for commit 19e1ffa deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app for commit 392f789 deployed to Heroku: https://hge-ci-pull-4922.herokuapp.com |
|
Review app https://hge-ci-pull-4922.herokuapp.com is deleted |
Description
This pr will allow console users to set sql function argument from session variable.
Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR.Affected components
Related Issues
close #4499
Solution and Design
Add UI to consume
track_function v2api https://hasura.io/docs/1.0/graphql/manual/api-reference/schema-metadata-api/custom-functions.html#track-function-v2.Steps to test and verify