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

Additional validation when changing the project language #3790

Merged
merged 11 commits into from May 22, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 13, 2018

Closes #3778

Copy link
Contributor

@agjohnson agjohnson left a comment

Great addition! I'm really happy we're able to address some of these problems around translations.

I noted a couple changes here, but looks good otherwise.

if main_project and main_project.language == language:
msg = (
'The translation can not have the same '
'language as the parent.'
Copy link
Contributor

@agjohnson agjohnson Mar 16, 2018

So two things:

  • We should also be testing for overlapping languages in sibling translations. That is, we should be checking for project.main_language_project.translations
  • I think we can reduce all the error messages to something more clear: There is already a {lang} translation for the {proj} project

self.assertIn(
'The translation can not have the same language',
''.join(form.errors['language'])
)
Copy link
Contributor

@agjohnson agjohnson Mar 16, 2018

Same here, we should test for sibling projects.

@stsewd stsewd force-pushed the validate-language-change branch from c6b5763 to 453be3b Mar 16, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 16, 2018

That test shouldn't pass, I'm going to add one more test

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 16, 2018

@agjohnson done, I forgot about checking the siblings.

Copy link
Member

@humitos humitos left a comment

Good!

Take a look at my comment regarding the form used. Besides the test, could you make a simple QA manually to check if it's working as you expect in your local instance?

This is one of the things that I don't like when only testing the forms and not the real request to the view. I'd like if you also add a test that make the real request to this private url.

.exclude(pk=project.pk)
.exists())
if siblings:
raise forms.ValidationError(format_msg)
Copy link
Member

@humitos humitos Mar 23, 2018

At this point, the project in the message, shouldn't be main_project?

Copy link
Member Author

@stsewd stsewd Mar 27, 2018

I'm not sure, all the messages here point to the current project. Should I change this?

Copy link
Member

@humitos humitos Apr 17, 2018

The question is, mentioning project will the messsage still be valid?

Which is the case of this validation error?

Copy link
Member Author

@stsewd stsewd Apr 17, 2018

I take this from #3790 (comment)

I think is kind of unnecessary to show the project here, right? Maybe just There is already a "{lang}" translation for this project. is enough, what do you think?

Copy link
Contributor

@agjohnson agjohnson Apr 18, 2018

I think mentioning the project is a good idea, I'm +1 on altering the message to reference main_project.

@@ -175,6 +175,29 @@ class Meta(object):
widget=forms.Textarea,
)

def clean_language(self):
Copy link
Member

@humitos humitos Mar 23, 2018

I think this is not the form you want.

ProjectExtraForm is used when importing a project via the Wizard. You need to write this logic in UpdateProjectForm which is the one used under https://readthedocs.org/dashboard/{project_slug}/edit/

Copy link
Member Author

@stsewd stsewd Mar 27, 2018

I was in doubt where to put it (in case it was reused on others parts). And since the ProjectExtraForm has the language field.

@stsewd stsewd force-pushed the validate-language-change branch from 99a23dc to 634dbec Mar 27, 2018
@stsewd stsewd force-pushed the validate-language-change branch from 9fcb9bb to 49e3744 Mar 27, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 27, 2018

@humitos done, I have added some test with manual requests. Also, I did a manual test in my instance, works as expected.

I just need a answer for #3790 (comment)

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 18, 2018

I agree with @humitos on the error message, otherwise this looks good to me! My original feedback has been addressed.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

@agjohnson done

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 22, 2018

This looks like a solid addition. Merging it. 👍

@ericholscher ericholscher merged commit 1d72ef5 into readthedocs:master May 22, 2018
1 check passed
@stsewd stsewd deleted the validate-language-change branch May 22, 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

5 participants