526469: No field for enableURL #9

Merged
merged 7 commits into from Aug 17, 2012

Projects

None yet

2 participants

@sergiocharpineljr

No description provided.

@bwinton bwinton commented on the diff Aug 16, 2012
ispdb/tests/test_issue.py
from urllib import quote_plus
from django.test import TestCase
from django.core.urlresolvers import reverse
from nose.tools import *
from ispdb.config import models
+from ispdb.tests.common import adding_domain_form
@bwinton
bwinton Aug 16, 2012

This line causes the tests to fail, because there is no common.py in this branch.

@bwinton bwinton commented on an outdated diff Aug 16, 2012
ispdb/config/models.py
+ def __str__(self): return str(self.url)
+ def __unicode__(self): return self.url
+
+
+class EnableURLInst(models.Model):
+ instruction = models.TextField(
+ max_length=100,
+ verbose_name="Instruction")
+ language = models.CharField(
+ max_length=10,
+ verbose_name="Language",
+ choices=settings.LANGUAGES)
+ enableurl = models.ForeignKey(EnableURL, related_name="instructions")
+
+ def __str__(self): return str(self.instruction)
+ def __unicode__(self): return self.instruction
@bwinton
bwinton Aug 16, 2012

I think we want to give these models and the DocURL/DocURLDesc models a common base class, since they're really quite similar.

@bwinton bwinton and 1 other commented on an outdated diff Aug 16, 2012
ispdb/config/models.py
super(DocURLDescForm, self).__init__(*args, **kwargs)
self.fields['description'].widget.attrs.update({'rows': 2, 'cols': 20})
+ choices = []
+ # add HTTP_ACCEPTED_LANG first
+ if http_accept_language:
+ codes = re.split(';|,', http_accept_language)
@bwinton
bwinton Aug 16, 2012

For reference, my accept-language is "en-US,en;q=0.8,en-ca;q=0.6,en-gb;q=0.4,en-us;q=0.2"

Based on that, I wonder if we should just split on ",", and then for each code throw away the ";q=.*" part…

Or, if we're going to use regexps, perhaps we should split on "(;q=[^,]*)?," (Let me know if that makes sense or not, okay? ;)

@sergiocharpineljr
sergiocharpineljr Aug 16, 2012

Yes it makes sense. I found the django method that parses it and I'm using it now, I think it safer.

@bwinton
bwinton Aug 17, 2012

Okay, I like that change, but I think we should set the first item as the selected item, instead of making me switch it off of "--------" every time…

@bwinton bwinton commented on an outdated diff Aug 16, 2012
ispdb/config/models.py
@@ -437,6 +478,174 @@ def delete(self):
desc.instance.delete()
form.instance.delete()
+
+class EnableURLInstForm(DynamicModelForm):
+ class Meta:
+ model = EnableURLInst
+ exclude = ['enableurl']
+
+ def __init__(self, *args, **kwargs):
+ http_accept_language = kwargs.pop('http_accept_language', '')
+ super(EnableURLInstForm, self).__init__(*args, **kwargs)
+ self.fields['instruction'].widget.attrs.update({'rows': 2, 'cols': 20})
+ choices = []
+ # add HTTP_ACCEPTED_LANG first
@bwinton
bwinton Aug 16, 2012

This also seems really similar to the code above, so putting it in a base class, too, seems like a reasonable thing to do.

@bwinton bwinton commented on an outdated diff Aug 16, 2012
ispdb/config/static/css/show_issue.css
@@ -63,6 +63,6 @@
float: right;
}
-.tab_desc {
+.tab_desc, .tab_inst {
@bwinton
bwinton Aug 16, 2012

Since I think we'll want the same styling on insts as we do on descs, why not just make a common class name and use that here instead?

@bwinton
bwinton Aug 16, 2012

(For that matter, can we re-use the same rendering for both of them?)

@bwinton bwinton commented on an outdated diff Aug 16, 2012
ispdb/templates/config/enter_config.html
+ $("#configform").on("click", "a.delete.inst", function(){
+ var prefix = $(this).attr('value');
+ var check = $('#id_'+prefix+'-DELETE');
+ if (check.val() == "True"){
+ check.val("False");
+ $('#id_'+prefix+'-instruction').removeAttr("disabled");
+ $('#id_'+prefix+'-language').removeAttr("disabled");
+ $(this).text("Remove");
+ } else {
+ check.val("True");
+ $('#id_'+prefix+'-instruction').attr("disabled", "disabled");
+ $('#id_'+prefix+'-language').attr("disabled", "disabled");
+ $(this).text("Undo");
+ }
+ });
+ $("#configform").on("click", "a.delete.enableurl", function(){
@bwinton
bwinton Aug 16, 2012

This event handler looks like something that could be merged into the on("click", "a.delete.desc") handler with some clever rearranging/renaming of code.

@bwinton
bwinton Aug 16, 2012

(Like setting some classes on the tds, and using $(this).sibling("td.whatever"), or something.)

@bwinton bwinton commented on an outdated diff Aug 16, 2012
ispdb/templates/config/enter_config.html
{{ config_form.docurl_formset.management_form }}
- <tr><td>{{ config_form.docurl_formset.non_form_errors }}</td></tr>
+ <tr><tdclass="noborder">{{ config_form.docurl_formset.non_form_errors }}</td></tr>
@bwinton
bwinton Aug 16, 2012

I'm pretty sure there should be a space in between "td" and "class". (Also on line 473.)

@bwinton bwinton commented on the diff Aug 16, 2012
ispdb/tests/test_issue.py
docurl = issue.config.docurl_set.all()[0]
- assert_equal("http://test2.com/", docurl.url)
@bwinton
bwinton Aug 16, 2012

I think we should use the reserved hostnames from the RFC, like example.com, so that we make sure we don't hit an actual server.

@sergiocharpineljr
sergiocharpineljr Aug 17, 2012

I wasn't worried about it because those tests use a separate db (empty one or those from fixtures attr), so there is no way to hit a real server.
But, if you think we should change them, I'll have to change almost all of the tests. So I think it's better to do it in the cleanup branch to ease this review.

@bwinton
bwinton Aug 17, 2012

That's fair.

@sergiocharpineljr sergiocharpineljr create a CommonLanguageDesc abstract class and a LanguageDescModelFor…
…m for DocURL and EnableURL classes, move common template code to different files for reuse and small fixes from pull request 9
7725792
@sergiocharpineljr

In the last commit I've created common model and form classes for DocURL and EnableURL. And moved its template code to ispdb/templates/config/urls/ dir to reuse them.
I know there is already some repeated code for nested formsets, but it will require more time to generalize it.

@bwinton bwinton merged commit 09b65b1 into mozilla:master Aug 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment