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

Fix the form for adopting a project #4721

Merged

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Oct 4, 2018

Fixes #4711

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 4, 2018

@stsewd Please review.

Copy link
Member

@stsewd stsewd left a comment

Thanks, but this isn't the solution we want. We need still need to request a project slug, because a project name isn't unique. What we want is to validate this on the form https://github.com/rtfd/readthedocs.org/blob/35695d170ec7e1dfa2553bf70bc406e7c960409a/readthedocs/gold/forms.py#L81-L96

And also add a test for this

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 4, 2018

@stsewd It's still unclear to me. This is what I am getting till now.
User must enter the project 'SLUG' in the field, if the project is not found, show error. And if the project is found, add it.
Is this the expected behaviour?

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 4, 2018

Yeah, that's it

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 4, 2018

@stsewd okay, Working on it.
But how would the user know that he/she has to enter the slug of the project?

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 4, 2018

I think right now we just have project as label. Maybe is worth to have a better label or helptext.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 4, 2018

@stsewd yeahh, adding a help text would be great. Some people might also not know the meaning of the SLUG, so it will be helpful.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 5, 2018

@stsewd Please review.

@@ -90,7 +91,13 @@ def __init__(self, *args, **kwargs):

def clean(self):
cleaned_data = super(GoldProjectForm, self).clean()
project_slug = cleaned_data.get('project', "")
Copy link
Member

@stsewd stsewd Oct 8, 2018

please use single quotes

self.add_error(None, 'You already have the max number of supported projects.')
# Checking if the project with the entered slug
# is present in the database or not
if Project.objects.filter(slug=project_slug).exists():
Copy link
Member

@stsewd stsewd Oct 8, 2018

Maybe this fits better on a clean_project method?

Copy link
Member Author

@dojutsu-user dojutsu-user Oct 8, 2018

yeahh... that will be better.
I have implemented it.

if Project.objects.filter(slug=project_slug).exists():
return cleaned_data
if project_slug:
self.add_error(None, "No project found.")
Copy link
Member

@stsewd stsewd Oct 8, 2018

Single quotes

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 8, 2018

@stsewd Please review.

Copy link
Member

@stsewd stsewd left a comment

Almost done, just some more changes!

project_instance = Project.objects.filter(slug=project_slug)

# Checking if the project with the entered slug
# is absent or present in the database.
Copy link
Member

@stsewd stsewd Oct 8, 2018

We don't need this comment, is clear what we are checking

Copy link
Member Author

@dojutsu-user dojutsu-user Oct 8, 2018

Working on it.

@@ -88,6 +89,18 @@ def __init__(self, *args, **kwargs):
self.projects = kwargs.pop('projects', None)
super(GoldProjectForm, self).__init__(*args, **kwargs)

def clean_project(self):
cleaned_data = super(GoldProjectForm, self).clean()
Copy link
Member

@stsewd stsewd Oct 8, 2018

You actually don't need to call to this method here, you can just use cleaned_data like this

https://github.com/rtfd/readthedocs.org/blob/90b1e52cfd6a124bf7298a1b3cb1711fddd67ab8/readthedocs/core/forms.py#L59-L65

Copy link
Member Author

@dojutsu-user dojutsu-user Oct 8, 2018

Wokring on it.

# Checking if the project with the entered slug
# is absent or present in the database.
if not project_instance.exists():
raise forms.ValidationError('No project found.')
Copy link
Member

@stsewd stsewd Oct 8, 2018

We need support translation for this message (using _(msg)) (like the above code)

Copy link
Member

@stsewd stsewd Oct 8, 2018

Yeah, that's it

Copy link
Member Author

@dojutsu-user dojutsu-user Oct 8, 2018

thanks.
though i deleted my comment before seeing your comment.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 8, 2018

@stsewd I have made the changes, please review it again.

stsewd
stsewd approved these changes Oct 9, 2018
Copy link
Member

@stsewd stsewd left a comment

Looks good, thanks! Just left one comment to improve the test.

@@ -26,6 +26,11 @@ def test_adding_projects(self):
self.assertEqual(self.golduser.projects.count(), 1)
self.assertEqual(resp.status_code, 302)

def test_incorrect_input_when_adding_projects(self):
self.assertEqual(self.golduser.projects.count(), 0)
resp = self.client.post(reverse('gold_projects'), data={'project': 'random-incorrect-slug'})
Copy link
Member

@stsewd stsewd Oct 9, 2018

Would be good to have an assert about that random-incorrect-slug doesn't exists on the db

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 9, 2018

@stsewd Added the improvement.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 11, 2018

@humitos Would it be worth it open an issue for the problem discussed in the comment above?
When I started to solve the issue, it does confuses me at first.
Please guide me.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 17, 2018

This looks good. 👍

@humitos Would it be worth it open an issue for the problem discussed in the comment above?

Sounds like a good issue to file.

@ericholscher ericholscher merged commit d06d47b into readthedocs:master Oct 17, 2018
1 check passed
@dojutsu-user dojutsu-user deleted the gold-member-add-project-form branch Dec 12, 2018
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

3 participants