-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
fix: Errors when saving dataset #24113
fix: Errors when saving dataset #24113
Conversation
@@ -150,7 +150,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({ | |||
groupby: column.groupby, | |||
is_active: column.is_active, | |||
is_dttm: column.is_dttm, | |||
python_date_format: column.python_date_format, | |||
python_date_format: column.python_date_format || null, |
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.
Allows for the field to be cleared, before an empty string was failing a Length(1, 255)
validation
)$ | ||
""", | ||
re.VERBOSE, | ||
) | ||
match = regex.match(value or "") | ||
if not match: | ||
raise ValidationError(_("Invalid date/timestamp format")) | ||
raise ValidationError([_("Invalid date/timestamp format")]) |
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.
The API error response was malformed without this, was giving ["I", "n", "v", ...]
. This fixes it
@@ -56,7 +56,7 @@ class DatasetColumnsPutSchema(Schema): | |||
filterable = fields.Boolean() | |||
groupby = fields.Boolean() | |||
is_active = fields.Boolean(allow_none=True) | |||
is_dttm = fields.Boolean(dump_default=False) | |||
is_dttm = fields.Boolean(allow_none=True, dump_default=False) |
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.
Columns can have is_dttm
being null, this allows datasets with such columns to be updated without error
Codecov Report
@@ Coverage Diff @@
## master #24113 +/- ##
=======================================
Coverage 68.30% 68.30%
=======================================
Files 1952 1952
Lines 75432 75433 +1
Branches 8191 8192 +1
=======================================
+ Hits 51521 51522 +1
Misses 21807 21807
Partials 2104 2104
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
SUMMARY
Recently, a PR changed the endpoint used to update datasets to
PUT /api/v1/dataset/{id}
: #23678The old endpoint did not have a lot of the validation the new endpoint had. As a result, some existing datasets would then fail validation once we switched over to the stricter one, making them unable to be updated unless you knew exactly what to change. It was also discovered that the custom validation for a column's
python_date_format
did not allow slashes in the date format, only hyphens, which was confusing since the UI placeholder uses slashes.This PR changes the validation to allow for either slashes or hyphens and makes a few other tweaks to reduce false-alarm validation errors.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION