Webhook notification URL size validation check #3680
Conversation
| if webhook_form.is_valid(): | ||
| webhook_form.save() | ||
| if 'email' not in request.POST: | ||
| email_form = EmailHookForm(data=None, project=project) |
There was a problem hiding this comment.
Why is data None here? and also the code above is doing the same (lines ~509 - 510)
There was a problem hiding this comment.
I am sorry @stsewd , I should have added a comment there. In the case that the URL is greater than max_length and email_form is empty the page should show only webhook_form.errors. If I don't reinitialize the email_form with None, the page shows errors for the email field along with the errors for the URL field.
A better way would be to handle each form separately, should I try to change the code handle both cases separately?
|
I have updated the code to display validation error for URL length. Can someone review this pull request? |
|
Thanks, @chirathr! We'll get to it when we can; Read The Docs is mostly volunteer run. Feel free to ping again in a week if this hasn't been done yet. |
|
|
||
| if request.method == 'POST': | ||
| if request.method == 'POST' and 'email' in request.POST: | ||
| email_form = EmailHookForm(data=request.POST or None, project=project) |
There was a problem hiding this comment.
We don't need the or None part here, since we already checking if the method is POST
There was a problem hiding this comment.
Thank you, I will remove that :)
| return HttpResponseRedirect(project_dashboard) | ||
| else: | ||
| # Blank email_form if webhook_form is submitted or the request is GET | ||
| email_form = EmailHookForm(data=None, project=project) |
There was a problem hiding this comment.
I think this part isn't needed, would be better to use an elif here to check for request.method == 'POST' and 'url' in request.POST:
There was a problem hiding this comment.
looks like this is needed for the below request.
There was a problem hiding this comment.
Yup, it is required. @stsewd should I change it to use and elif like you said?
There was a problem hiding this comment.
Would be awesome to do something like that (for readability) but keeping the same logic. Anyway, is fine how it is now, I just didn't read the rest of the code when commenting that 😁.
chirathr
force-pushed
the
webhook_notifications_url_size
branch
from
33e03b3 to
d9f4c17
Compare
chirathr
force-pushed
the
webhook_notifications_url_size
branch
4 times, most recently
from
92925af to
8550754
Compare
|
Thank @humitos. I have made the changes that were requested. Running |
|
Testing this locally and it still doesn't properly do validation. It is saving the object in |
ericholscher
force-pushed
the
webhook_notifications_url_size
branch
from
8550754 to
4b0ddf1
Compare
Codecov Report
@@ Coverage Diff @@
## master #3680 +/- ##
==========================================
+ Coverage 76.46% 76.47% +<.01%
==========================================
Files 158 158
Lines 9997 9997
Branches 1262 1262
==========================================
+ Hits 7644 7645 +1
+ Misses 2011 2010 -1
Partials 342 342
|
…tifications_url_size
…r-webhook_notifications_url_size
…r-webhook_notifications_url_size
This PR is regarding issue #3669
I have fixed the issue with the form not validating the URL size and also added
max_lengthin the form to prevent input from being larger than 600 characters.I had to add conditions to check which form is submitted, as there are two forms in the template
projects/project_notifications.html. Maybe we could change it to a single form or separate out the views that handle each POST request?Please let me know if I made some mistakes.