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

Projects
None yet
5 participants
@stsewd
Member

stsewd commented Mar 13, 2018

Closes #3778

@agjohnson

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.'

This comment has been minimized.

@agjohnson

agjohnson Mar 16, 2018

Contributor

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'])
)

This comment has been minimized.

@agjohnson

agjohnson Mar 16, 2018

Contributor

Same here, we should test for sibling projects.

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 16, 2018

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

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 16, 2018

@agjohnson done, I forgot about checking the siblings.

@humitos

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)

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

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

This comment has been minimized.

@stsewd

stsewd Mar 27, 2018

Member

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

This comment has been minimized.

@humitos

humitos Apr 17, 2018

Member

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

Which is the case of this validation error?

This comment has been minimized.

@stsewd

stsewd Apr 17, 2018

Member

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?

This comment has been minimized.

@agjohnson

agjohnson Apr 18, 2018

Contributor

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

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

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/

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

class ProjectUpdate(ProjectSpamMixin, PrivateViewMixin, UpdateView):
form_class = UpdateProjectForm
model = Project
success_message = _('Project settings updated')
template_name = 'projects/project_edit.html'
lookup_url_kwarg = 'project_slug'
lookup_field = 'slug'
def get_queryset(self):
return self.model.objects.for_admin_user(self.request.user)
def get_success_url(self):
return reverse('projects_detail', args=[self.object.slug])

This comment has been minimized.

@stsewd

stsewd Mar 27, 2018

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

stsewd commented Apr 18, 2018

@agjohnson done

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 22, 2018

This looks like a solid addition. Merging it. 👍

@ericholscher ericholscher merged commit 1d72ef5 into rtfd:master May 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd: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