Skip to content
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

Validate url from webhook notification #4983

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Dec 10, 2018

Fixes #4981

@codecov
Copy link

@codecov codecov bot commented Dec 10, 2018

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

Copy link
Member

@humitos humitos left a comment

Thanks for working on this.

I suggested a different approach for this.

readthedocs/projects/forms.py Show resolved Hide resolved
@dojutsu-user dojutsu-user changed the title [WIP] Validate url from webhook notification Validate url from webhook notification Dec 11, 2018
Copy link
Member

@humitos humitos left a comment

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 💯

)
return HttpResponseRedirect(project_dashboard)
if 'email' in request.POST.keys():
email_form = EmailHookForm(data=request.POST, project=project)
Copy link
Member

@humitos humitos Dec 11, 2018

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.

Copy link
Member Author

@dojutsu-user dojutsu-user Dec 11, 2018

That raises this type of situation
screenshot from 2018-12-11 19-59-26

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

Copy link
Member

@humitos humitos Dec 11, 2018

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.

Copy link
Member Author

@dojutsu-user dojutsu-user Dec 11, 2018

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

Copy link
Member

@humitos humitos Dec 11, 2018

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.

Copy link
Member Author

@dojutsu-user dojutsu-user Dec 11, 2018

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

readthedocs/rtd_tests/tests/test_project_forms.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member

@humitos humitos commented Dec 11, 2018

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.

Copy link
Member

@humitos humitos left a comment

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

@dojutsu-user dojutsu-user force-pushed the validate-url-from-webhook-notification branch 2 times, most recently from 7118a41 to 8e314dd Jan 17, 2019
@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 17, 2019

@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?

Copy link
Member

@ericholscher ericholscher left a comment

Looks good. 👍

@ericholscher ericholscher merged commit 621467d into readthedocs:master Jan 22, 2019
1 of 2 checks passed
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 22, 2019

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 :/

@dojutsu-user dojutsu-user deleted the validate-url-from-webhook-notification branch Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants