Switch ZendeskForm to ModelForm#7265
Conversation
denyshon
force-pushed
the
zendeskfrom-to-modelform
branch
4 times, most recently
from
731da2d to
5a3a57e
Compare
akatsoulas
left a comment
akatsoulas
left a comment
There was a problem hiding this comment.
Thanks @denyshon for the PR! I added a couple of comment
denyshon
force-pushed
the
zendeskfrom-to-modelform
branch
2 times, most recently
from
6c2d272 to
7f286c9
Compare
|
@akatsoulas Thank you for the review! I've replied to your comment and rebased the patch onto the latest main. |
denyshon
force-pushed
the
zendeskfrom-to-modelform
branch
from
7f286c9 to
410e1ec
Compare
| description = models.TextField() | ||
| category = models.CharField(max_length=255) | ||
| description = models.TextField(max_length=65563) | ||
| category = models.CharField(max_length=255, blank=True, default="") |
There was a problem hiding this comment.
Given that the form does not allow an empty value, I would argue that the model shouldn't allow this as well.
There was a problem hiding this comment.
I suspect this change will also need a schema migration
There was a problem hiding this comment.
form does allow an empty value: category=self.cleaned_data.get("category", "")
in case we'll unmark the field as required
There was a problem hiding this comment.
I have a handwritten migration, but from what I understand, it should ideally be generated by django?
There was a problem hiding this comment.
(updated the comment above, the current behavior regarding an empty value is not consistent, so you'll be the best person to decide on this)
There was a problem hiding this comment.
you can run ./manage.py makemigrations to create the migration and ./manage migrate to apply it
| subject = models.CharField(max_length=255) | ||
| description = models.TextField() | ||
| category = models.CharField(max_length=255) | ||
| description = models.TextField(max_length=65563) |
There was a problem hiding this comment.
Shouldn't the value be 65535?
There was a problem hiding this comment.
I believe I took that value from the docs, but now they say just "64 kB", so let me make change this to 65535
| } | ||
| # An empty category field will result in a form validation error in send()->save(). | ||
| # Since we don't care about the form, we'll just clear the errors to run send(). | ||
| form._errors = "" |
There was a problem hiding this comment.
I am not sure why we are bypassing the form errors here. We shouldn't be able to send() given the invalid form
There was a problem hiding this comment.
we can pass a random topic if you want
Switch ZendeskForm from Form to ModelForm to benefit from model validation. Specify SupportTicket.description max_length based on the Zendesk API. Set blank=True for SupportTicket.category to prevent a model validation error in case we decide to make this field optional. Adjust kitsune.customercare.tests.test_forms.ZendeskFormTests.test_send_with_no_category_selected to ignore a form validation error.
denyshon
force-pushed
the
zendeskfrom-to-modelform
branch
from
410e1ec to
189f314
Compare
2 participants
max_lengthbased on the Zendesk API.blank=Truefor SupportTicket.category to prevent a model validation error in case we decide to make this field optional.Resolves #2806.