-
Notifications
You must be signed in to change notification settings - Fork 25
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
Define date range of the date questions #2081
base: main
Are you sure you want to change the base?
Conversation
b04b2bc
to
cfc05d9
Compare
cfc05d9
to
27d90a9
Compare
27d90a9
to
77916a9
Compare
@MitanOmar is this PR ready to merge? Are you gonna add validation for the date range in the answers as well, or is this intended more as a range limiter for the input field? |
@@ -240,12 +240,28 @@ class Meta(SaveQuestionSerializer.Meta): | |||
|
|||
|
|||
class SaveDateQuestionSerializer(SaveQuestionSerializer): | |||
min_date = DateField("%Y-%m-%d", required=False, allow_null=True) | |||
max_date = DateField("%Y-%m-%d", required=False, allow_null=True) | |||
|
|||
def validate(self, data): |
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.
@winged there is a validation in the backend, but since i finish this PR, it's waiting for review
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.
projectcaluma/ember-caluma#2521
here is the implementation of the frontend side
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.
Right, but I think if we do this, we should add validation to the answers as well. This only validates the question, but no restriction happens when people actually fill out the forms.
Look at the answer validator to see what I mean.
There's already _validate_question_float()
and _validate_question_integer()
that implement the same pattern. I think therefore, you should extend _validate_question_date()
to match.
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.
that's right, i will implement that too, and i will include it in my todo for this week
As discussed today: Since this doesn't stem from a direct (client) requirement, we will stop development for now, and pick it back up when we do have a need for this feature. Otherwise, we may have a breaking change to modify this in a way that actually benefits the user. Ideas that may be useful in this context, that aren't possible with the current approach:
These limits may possibly also be interesting for float / integer questions as well, but as we're not clear on any actual requirements, let's not run off in the wrong direction here. |
No description provided.