Validate url from webhook notification

[dojutsu-user]

Fixes #4981

[codecov]

Codecov Report

Merging #4983 into master will decrease coverage by 0.18%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##           master    #4983      +/-   ##
==========================================
- Coverage   77.04%   76.86%   -0.19%     
==========================================
  Files         158      158              
  Lines       10108     9947     -161     
  Branches     1274     1245      -29     
==========================================
- Hits         7788     7646     -142     
+ Misses       1989     1968      -21     
- Partials      331      333       +2

Impacted Files	Coverage Δ
readthedocs/projects/forms.py	81.86% <100%> (+1.46%)	⬆️
readthedocs/projects/views/private.py	79.63% <20%> (-0.77%)	⬇️
readthedocs/vcs_support/backends/svn.py	29.5% <0%> (-9.89%)	⬇️
readthedocs/core/utils/__init__.py	74.73% <0%> (-5.68%)	⬇️
readthedocs/vcs_support/backends/hg.py	64.51% <0%> (-3.18%)	⬇️
readthedocs/notifications/storages.py	82.53% <0%> (-3.18%)	⬇️
readthedocs/doc_builder/config.py	91.89% <0%> (-2.56%)	⬇️
readthedocs/projects/querysets.py	80.58% <0%> (-1.07%)	⬇️
readthedocs/core/views/serve.py	87.25% <0%> (-0.99%)	⬇️
readthedocs/projects/exceptions.py	84.21% <0%> (-0.79%)	⬇️
... and 31 more

[humitos]

Thanks for working on this.

I suggested a different approach for this.

[humitos]

This is good.

I'd like a rework on the view to keep it simple (similar as it was). After this change is done, we can merge it.

I like the tests 💯

[humitos]

You can leave the all this code as it was, but calling the is_valid method on the right form only after checking request.POST.keys.

[dojutsu-user]

That raises this type of situation

I have submitted empty form for Webhook Notification URL, but it is also showing error for Email Field.

[humitos]

This is the flow to me:

if 'url' in request.POST.keys() and webhook_form.is_valid():
    webhook_form.save()
if 'email' in request.POST.keys() and email_form.is_valid():
    email_form.save()

The rest of the code should be as it was before.

[dojutsu-user]

This will also give rise to the same situation (tried on local server).
This is what is happening

>>> from readthedocs.projects.forms import WebHookForm
>>> from readthedocs.projects.models import Project
>>> proj = Project.objects.get(slug='pikachu-demo')
>>> data = {'url': ''}
>>> form = WebHookForm(data=data, project=proj)
>>> form
<WebHookForm bound=True, valid=Unknown, fields=(url)>
>>> form.errors
{'url': ['This field is required.']}
>>> form
<WebHookForm bound=True, valid=False, fields=(url)>

So with these lines

    email_form = EmailHookForm(data=request.POST or None, project=project)
    webhook_form = WebHookForm(data=request.POST or None, project=project)

even the user hasn't submitted the email_form, the error will rise and will be shown in the templates even if the is_valid() method is not run

[humitos]

even the user hasn't submitted the email_form, the error will rise and will be shown in the templates even if the is_valid() method is not run

Really?! Wow! I didn't know that and it's an unexpected behavior to me.

[dojutsu-user]

I think, it's because email form will then accept data=request.POST,
but yeahh.. it's unexpected the error is generated without calling is_valid() method

[humitos]

I suppose that we can manually delete these from .org db once this PR gets merged,

In [2]: WebHook.objects.filter(url='').count()
Out[2]: 20

These are useless and just produce our code to fail.

[humitos]

I think we are fine merging this once conflicts are solved and all the tests pass.

[dojutsu-user]

@humitos
I have fixed the merge conflicts.

Edit:
Codecov is failing because of the editing of the view (project_notifications). Are the tests required for that?

[ericholscher]

Looks good. 👍

[ericholscher]

Codecov is failing because of the editing of the view (project_notifications). Are the tests required for that?

No. I want to probably turn that check off, it's annoying :/