-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Parameter feedback - #1 Server errors #4312
base: master
Are you sure you want to change the base?
Conversation
e6214cc
to
26c80a0
Compare
26c80a0
to
ac77587
Compare
redash/models/parameterized_query.py
Outdated
|
||
definition = next((definition for definition in self.schema if definition["name"] == name), None) | ||
|
||
if not definition: | ||
return False | ||
return 'Parameter no longer exists in query.' |
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.
Kept as safety but can not be reached since no-definition params are filtered out beforehand.
redash/handlers/query_results.py
Outdated
def error_response(message, http_status=400): | ||
return {'job': {'status': 4, 'error': message}}, http_status | ||
def error_response(message, data=None, http_status=400): | ||
return {'job': {'status': 4, 'error': message, 'error_data': data}}, http_status |
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 called it error_data
but lmk if there's a standard naming convention.
@ranbena , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts? We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that. |
This seems like another useful PR that would be good to merge. 😄 |
* Parameter feedback - #2 Client errors in query page * Added cypress test * Fixed percy screenshot * Safer touched change * Parameter feedback - #3 Added in Widgets (#4320) * Parameter feedback - #3 Added in Widgets * Added cypress tests * Making sure widget-level param is selected * Parameter feedback - #4 Added in Dashboard params (#4321) * Parameter feedback - #4 Added in Dashboard params * Added cypress test * Moved to service * Parameter feedback - #5 Unsaved indication (#4322) * Parameter feedback - #5 Unsaved indication * Added ANGULAR_REMOVE_ME * Added cypress test * Fixed percy screenshot * Some code improvements * Parameter input feedback - #6 Better value normalization (#4327)
* Restyled by autopep8 * Restyled by black * Restyled by clang-format * Restyled by isort * Restyled by prettier * Restyled by reorder-python-imports * Restyled by whitespace * Restyled by yapf --------- Co-authored-by: Restyled.io <commits@restyled.io>
Description
This is part 1 of "Parameter feedback" feature discussed in https://discuss.redash.io/t/query-parameter-validation-and-feedback/4759.
Changed parameter error feedback from server to be more descriptive and in preparation for showing error indication on parameter UI. It now provides per-parameter error messages.
One parameter empty
Before (client):
After:
Two parameters empty
Before (client):
After:
Text parameter has numeric value
Before:
After:
Text parameter has date range query tags
Before:
After:
Data range param doesn't have expected query tags
Before:
After:
Parameter name in schema not found in parameters
Before:
After:
Two parameter names in schema not found in sent parameters
Before:
After:
Parameter not in schema
Before:
After:
Ignored. Query continues to execution.