Skip to content

Bug 526468 and Bug 518319: Allow documentation pages in more languages #8

Merged
merged 9 commits into from Aug 15, 2012

2 participants

@sergiocharpineljr
  • Bug 526468: Can't enter several documentation URLs
  • Bug 518319: Allow documentation pages in more languages
  • move complexity of the views to form classes
  • create base classes to work with our dynamic form JS code

PS: on the cleanup branch I've moved the form classes to forms.py and made those files pep8 compatible.

@bwinton bwinton commented on the diff Aug 14, 2012
ispdb/config/models.py
from django.utils.safestring import mark_safe
from django.contrib.auth.models import User
+from django.conf import settings
+from django.utils.translation import get_language_info
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

Please keep the imports in alphabetical order. (Unless you've already done that in your cleanup branch.)

I'll do it in cleanup branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton and 1 other commented on an outdated diff Aug 14, 2012
ispdb/config/models.py
@@ -185,6 +187,98 @@ class Issue(models.Model):
status = models.CharField(max_length=20, choices=STATUS_CHOICES,
default="open")
+class DocURL(models.Model):
+ url = models.URLField(
+ verbose_name="URL of the page describing these settings")
+ config = models.ForeignKey(Config)
+
+ def __str__(self): return str(self.url)
+ def __unicode__(self): return self.url
+
+class DocURLDesc(models.Model):
+ language = models.CharField(
+ max_length=10,
+ verbose_name="Language",
+ choices=settings.LANGUAGES)
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

I think we should default this to English.

blank and null values are not permitted. And on bug_526469 branch I've added HTTP_ACCEPT_LANGUAGE languages to the top of the choices list (which will be the default). Do you think we still need a default value here?

@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

No, I think having the accept language be the default will be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/config/models.py
+ url = models.URLField(
+ verbose_name="URL of the page describing these settings")
+ config = models.ForeignKey(Config)
+
+ def __str__(self): return str(self.url)
+ def __unicode__(self): return self.url
+
+class DocURLDesc(models.Model):
+ language = models.CharField(
+ max_length=10,
+ verbose_name="Language",
+ choices=settings.LANGUAGES)
+ description = models.CharField(
+ max_length=100,
+ verbose_name="Description of the settings page")
+ docurl = models.ForeignKey(DocURL, related_name="descriptions")
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

And I think we want the description to come before the language, since that kind of makes more sense to me… I'm not sure I can explain why, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on the diff Aug 14, 2012
ispdb/config/models.py
+ continue
+ # discard deleted new forms
+ if form.cleaned_data['DELETE'] and not form in self.initial_forms:
+ continue
+ form.save(*args, **kwargs)
+
+ def _get_empty_form(self, **kwargs):
+ defaults = {
+ 'auto_id': 'id_%s',
+ 'prefix': '{1}-{0}',
+ 'empty_permitted': True,
+ }
+ defaults.update(kwargs)
+ form = self.form(**defaults)
+ return form
+ empty_form = property(_get_empty_form)
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

I think we should move these out to a dynamicModels.py file, which we import in models.py.

on the cleanup branch I've moved all of this forms to a forms.py file. Do you still think we should move these two classes to another file (possibly in another dir as well, something like ispdb/common/forms/ dir) ? Or can we let them on forms.py file?

@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

Let's leave them in forms.py for now, and if I think it's too large, I'll mention it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on the diff Aug 14, 2012
ispdb/config/models.py
@@ -241,27 +463,58 @@ def clean(self):
msg = ("Domain name is not valid")
try:
if not regex.match(data.encode('idna')): # IDN -> ACE
- raise ValidationError(mark_safe(msg))
+ raise ValidationError(msg)
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

Why aren't we marking this safe anymore?

Because there is not html tags in msg, so there is no need to mark it safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/config/templatetags/custom_filters.py
@@ -7,7 +7,7 @@
@register.filter
def data_verbose(boundField, attr_name="value"):
"""
- Returns field's data or it's verbose version
+ Returns field's data or it's verbose version
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

nit: "its", not "it's" (Also on line 28.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on the diff Aug 14, 2012
ispdb/fixtures/xml_testdata.json
+ "pk": 1,
+ "model": "config.docurl",
+ "fields": {
+ "url": "http://test.com/",
+ "config": 1
+ }
+ },
+ {
+ "pk": 1,
+ "model": "config.docurldesc",
+ "fields": {
+ "docurl": 1,
+ "description": "test",
+ "language": "en"
+ }
+ }
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

I think it might be useful to have more than one description for a docurl, and more than one docurl for a config in our test data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/templates/config/details.html
@@ -260,6 +260,21 @@
</tr>
{% endfor %}
</table>
+ {% for docurl in docurls %}
+ <table>
+ <tr>
+ <td>{{ docurl.url.label}}:</td><td>{{ docurl.url.value}}</td>
+ </tr>
+ </table>
+ {% for desc in docurl.desc_formset %}
+ <table class="tab_desc">
+ <tr>
+ <td>{{ desc.language.label }}:</td><td>{{ desc.language|data_verbose_field }}</td>
+ <td>{{ desc.description.label }}:</td><td>{{ desc.description.value }}</td>
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

Yeah, I think we can dispense with "desc.language.label", since it should be obvious from the content of the field…
And moving it to the end of the description line seems like a good idea, too.

Something like:
{{ desc.description.label }}:{{ desc.description.value }} {{ desc.language|data_verbose_field }}

@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

(Uh, like that, only with tags in the middle. :P )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/templates/config/enter_config.html
- function addRow() {
- $(template(i++)).appendTo("#domain_list table");
- $("#id_form-TOTAL_FORMS").val(i);
+ // add functions
+ function addRow(prefix, template_selector, appendto_selector) {
+ var template = jQuery.format($(template_selector).val());
+ var i = $("#id_"+prefix+"-TOTAL_FORMS").val();
+ if (i === undefined){
+ i = 0;
+ }
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

I think we could write this as:
var i = $("#id_"+prefix+"-TOTAL_FORMS").val() | 0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/templates/config/enter_config.html
+ addRow('{{config_form.domain_formset.prefix}}', '#domain_template', '#domain_list table');
+ });
+ $("#configform").on("click", "a.add_desc", function() {
+ var prefix = $(this).attr('value');
+ addRow(prefix, '#desc_template', '#'+prefix+' table.tab_descs');
+ });
+
+ // delete functions
+ $("#domain_list").on("click", "a.delete", function(){
+ var prefix = $(this).attr('value');
+ var check = $('#id_'+prefix+'-DELETE');
+ if (check.val() == "True"){
+ check.val("False");
+ $('#id_'+prefix+'-name').removeAttr("readonly");
+ $(this).text("Remove");
+ }else{
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

nit: spaces around the else, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton bwinton commented on an outdated diff Aug 14, 2012
ispdb/templates/config/enter_config.html
</td>
- <td><a class="link delete_domain" value="{0}">Remove</a>
+ </tr>
+{% endwith %}
+</textarea>
+<textarea style="display:none" id="desc_template">
+{% with config_form.docurl_formset.empty_desc_form as form %}
+ <tr>
+ <th>{{ form.description.label_tag }}:</th>
+ <td>{{ form.language }}</td>
+ <td>
+ {{ form.description }}
+ {{ form.DELETE }}
@bwinton
Mozilla member
bwinton added a note Aug 14, 2012

Here, too, I think we should put the language after the description…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwinton
Mozilla member
bwinton commented Aug 14, 2012

Okay, I think that's all from me. Thanks!

@sergiocharpineljr

Done. Thanks!

@bwinton bwinton merged commit 5f16017 into mozilla:master Aug 15, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.