Fixes 863381 - Creates a system for internal redirects of products inside Bouncer. #28

Merged
merged 1 commit into from Aug 27, 2013

Projects

None yet

4 participants

@brandonsavage
Contributor

No description provided.

@brandonsavage
Contributor

DO NOT MERGE; I will take care of the merge process once we have done the robots.txt release.

@brandonsavage
Contributor

@lauraxt r? on PHP, @pmclanahan r? on python.

@lauraxt lauraxt and 2 others commented on an outdated diff Aug 15, 2013
apps/api/views.py
@@ -357,6 +357,44 @@ def location_delete(request):
return xml.success('Deleted location: %s' % location)
+@require_POST
+@csrf_exempt
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):
+ """Create or update an alias for a product"""
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
@lauraxt
lauraxt Aug 15, 2013 Contributor

Having the error numbers hardcoded in multiple places is a code smell.

@lauraxt
lauraxt Aug 15, 2013 Contributor

(Try and define these somewhere with meaningful names and use those instead)

@pmclanahan
pmclanahan Aug 16, 2013 Member

I 2nd this. Something like:

class ERRORS:
    alias_required = 101
    product_required = 102
    product_invalid = 103
    alias_invalid = 104

Or go even simpler and make them all constants:

E_ALIAS_REQUIRED = 101
E_PRODUCT_REQUIRED = 102
...
@brandonsavage
brandonsavage Aug 16, 2013 Contributor

I agree, though the code I wrote is consistent with the code in the rest of the view. A follow up bug can be filed to resolve the code smell at a later date.
On Aug 16, 2013, at 4:15 PM, Paul McLanahan notifications@github.com wrote:

In apps/api/views.py:

@@ -357,6 +357,44 @@ def location_delete(request):
return xml.success('Deleted location: %s' % location)

+@require_POST
+@csrf_exempt
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):

  • """Create or update an alias for a product"""
  • xml = XMLRenderer()
  • alias = request.POST.get('alias', None)
  • redirect = request.POST.get('related_product', None)
  • if not alias:
  •    return xml.error('alias name is required.', errno=101)
    
    I 2nd this. Something like:

class ERRORS:
alias_required = 101
product_required = 102
product_invalid = 103
alias_invalid = 104
Or go even simpler and make them all constants:

E_ALIAS_REQUIRED = 101
E_PRODUCT_REQUIRED = 102
...

Reply to this email directly or view it on GitHub.

@lauraxt
lauraxt Aug 19, 2013 Contributor

I would honestly prefer to fix it now - this part is pretty self contained

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/api/views.py
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):
+ """Create or update an alias for a product"""
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ try:
+ exists = Product.objects.get(name=redirect)
+ except:
@pmclanahan
pmclanahan Aug 15, 2013 Member

non-specific except blocks should be avoided. This should be except Product.DoesNotExist:

@lauraxt lauraxt commented on an outdated diff Aug 15, 2013
bouncer/php/index.php
@@ -58,6 +58,28 @@
);
$sdo = new SDO2($mc, $dbwrite);
+
+ // New alias code
@lauraxt
lauraxt Aug 15, 2013 Contributor

non useful comment

@lauraxt lauraxt and 1 other commented on an outdated diff Aug 15, 2013
bouncer/php/index.php
@@ -58,6 +58,28 @@
);
$sdo = new SDO2($mc, $dbwrite);
+
+ // New alias code
+ $alias_sql = 'SELECT * FROM mirror_aliases WHERE alias = ?';
+ $alias = $sdo->get_one($alias_sql, array($product_name), SDO2::FETCH_ASSOC);
@lauraxt
lauraxt Aug 15, 2013 Contributor

weird way to do a LIMIT 1 but ok

@brandonsavage
brandonsavage Aug 15, 2013 Contributor

Since the alias name is unique, there can't ever be more than one, so a LIMIT 1 would be redundant in this case.

On Aug 15, 2013, at 10:17 AM, Laura Thomson notifications@github.com wrote:

In bouncer/php/index.php:

@@ -58,6 +58,28 @@
);
$sdo = new SDO2($mc, $dbwrite);

  • // New alias code
  • $alias_sql = 'SELECT * FROM mirror_aliases WHERE alias = ?';
  • $alias = $sdo->get_one($alias_sql, array($product_name), SDO2::FETCH_ASSOC);
    weird way to do a LIMIT 1 but ok


Reply to this email directly or view it on GitHub.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -78,6 +80,16 @@ class Meta:
db_table = 'mirror_products'
+class ProductAlias(models.Model):
+ """An alias for product models"""
+ id = models.AutoField(primary_key=True)
@pmclanahan
pmclanahan Aug 15, 2013 Member

This is implied with all Django models.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/forms.py
class UptakeForm(forms.Form):
p = forms.ModelMultipleChoiceField(
queryset=Product.objects.order_by('name'),
label='Products')
+class ProductAliasAdminForm(forms.ModelForm):
+ class Meta:
+ model = ProductAlias
+
+ def clean_alias(self):
+ try:
+ Product.objects.get(name=self.cleaned_data['alias'])
+ except:
+ return self.cleaned_data['alias']
+ raise forms.ValidationError("Your alias cannot share the same name as an existing product!")
+
+
+
@pmclanahan
pmclanahan Aug 15, 2013 Member

One blank line between methods.

@pmclanahan pmclanahan and 1 other commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -78,6 +80,16 @@ class Meta:
db_table = 'mirror_products'
+class ProductAlias(models.Model):
+ """An alias for product models"""
+ id = models.AutoField(primary_key=True)
+ alias = models.CharField(max_length=255, unique=True)
+ related_product=models.CharField(max_length=255, unique=False)
@pmclanahan
pmclanahan Aug 15, 2013 Member

Why not use a ForeignKey here?

@pmclanahan
pmclanahan Aug 15, 2013 Member

Also space around the equals sign.

@brandonsavage
brandonsavage Aug 15, 2013 Contributor

The choice not to use a foreign key here was to make the PHP script run a bit faster by not using a join, and to eliminate the 2300 record dropdown from the admin section.

@pmclanahan
pmclanahan Aug 16, 2013 Member

Ah. Yes. Though modifying the admin form to use a text field instead of a select isn't that hard if you still wanted to use a real relationship. I'm sure queries on the PHP side could still be crafted for speed. Up to you though.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -78,6 +80,16 @@ class Meta:
db_table = 'mirror_products'
+class ProductAlias(models.Model):
+ """An alias for product models"""
+ id = models.AutoField(primary_key=True)
+ alias = models.CharField(max_length=255, unique=True)
@pmclanahan
pmclanahan Aug 15, 2013 Member

Might consider db_index=True here.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/forms.py
class UptakeForm(forms.Form):
p = forms.ModelMultipleChoiceField(
queryset=Product.objects.order_by('name'),
label='Products')
+class ProductAliasAdminForm(forms.ModelForm):
+ class Meta:
+ model = ProductAlias
+
+ def clean_alias(self):
+ try:
+ Product.objects.get(name=self.cleaned_data['alias'])
+ except:
+ return self.cleaned_data['alias']
+ raise forms.ValidationError("Your alias cannot share the same name as an existing product!")
+
+
+
+ def clean_related_product(self):
+ try:
@pmclanahan
pmclanahan Aug 15, 2013 Member

This would be more efficient as an exists() query as it doesn't fetch the whole record nor create an object:

if not Product.objects.filter(name=self.cleaned_data['related_product']).exists():
    raise forms.ValidationError("The product you entered was invalid")

return self.cleaned_data['related_product']
@lauraxt lauraxt commented on an outdated diff Aug 15, 2013
bouncer/php/index.php
- $redirect_url = WEBPATH . '?product=firefox-' . $latest .
+ // New alias code
+ $alias_sql = 'SELECT * FROM mirror_aliases WHERE alias = ?';
+ $alias = $sdo->get_one($alias_sql, array($product_name), SDO2::FETCH_ASSOC);
+ if($alias) {
+ $product_name = $alias['related_product'];
+
+ $redirect_url = WEBPATH . '?product=' . $product_name .
'&os=' . $os_name . '&lang=' . $where_lang;
@lauraxt
lauraxt Aug 15, 2013 Contributor

Once we have the actual product name here, there is no need to redirect. We can just proceed to finding a valid mirror at this point.

@lauraxt lauraxt commented on an outdated diff Aug 15, 2013
apps/api/views.py
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ try:
+ exists = Product.objects.get(name=redirect)
+ except:
+ return xml.error('You cannot create an alias for a product that does not exist', errno=103)
+
+ try:
+ Product.objects.get(name=alias)
+ return xml.error('You cannot create an alias with the same name as a product', errno=104)
@lauraxt
lauraxt Aug 15, 2013 Contributor

I really dislike this idiom, fwiw

@lauraxt
lauraxt Aug 15, 2013 Contributor

And to be clear, I mean where the except block is the expected behavior, and the mainline is the exception. Yuck.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/api/views.py
+@csrf_exempt
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):
+ """Create or update an alias for a product"""
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ try:
+ exists = Product.objects.get(name=redirect)
@pmclanahan
pmclanahan Aug 15, 2013 Member

Don't seem to be using this variable. This is another good candidate for an exists() query.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/api/views.py
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ try:
+ exists = Product.objects.get(name=redirect)
+ except:
+ return xml.error('You cannot create an alias for a product that does not exist', errno=103)
+
+ try:
+ Product.objects.get(name=alias)
@pmclanahan
pmclanahan Aug 15, 2013 Member

I agree with @lauraxt here. I'd prefer:

if Product.objects.filter(name=alias).exists():
    return xml.error(...)
@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/api/views.py
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ try:
+ exists = Product.objects.get(name=redirect)
+ except:
+ return xml.error('You cannot create an alias for a product that does not exist', errno=103)
+
+ try:
+ Product.objects.get(name=alias)
+ return xml.error('You cannot create an alias with the same name as a product', errno=104)
+ except:
+ pass # If the alias is not the same as a product, then we're fine.
+
+ try:
@pmclanahan
pmclanahan Aug 15, 2013 Member

I believe this could all just be a get_or_create:

alias_obj, created = ProductAlias.objects.get_or_create(alias=alias, defaults={'related_product': redirect})
if not created:
    # assuming you want to update the redirect if it already exists.
    alias_obj.related_product = redirect
    alias_obj.save()
@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/forms.py
class UptakeForm(forms.Form):
p = forms.ModelMultipleChoiceField(
queryset=Product.objects.order_by('name'),
label='Products')
+class ProductAliasAdminForm(forms.ModelForm):
+ class Meta:
+ model = ProductAlias
+
+ def clean_alias(self):
+ try:
@pmclanahan
pmclanahan Aug 15, 2013 Member

Use an exists query here as well.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -182,3 +194,13 @@ class Meta:
def __unicode__(self):
return u"%s (%s)" % (unicode(self.lmm), self.lang)
+
+@receiver(pre_save, sender=ProductAlias)
+def check_valid_product(sender, instance, **kwargs):
+
+ try:
+ Product.objects.get(name=instance.related_product);
+ except:
+ instance.related_product = None
@pmclanahan
pmclanahan Aug 15, 2013 Member

This should make the save fail since the field doesn't allow Null. Is that what you intend?

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -182,3 +194,13 @@ class Meta:
def __unicode__(self):
return u"%s (%s)" % (unicode(self.lmm), self.lang)
+
+@receiver(pre_save, sender=ProductAlias)
+def check_valid_product(sender, instance, **kwargs):
+
+ try:
@pmclanahan
pmclanahan Aug 15, 2013 Member

Another good exists() spot.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/mirror/models.py
@@ -182,3 +194,13 @@ class Meta:
def __unicode__(self):
return u"%s (%s)" % (unicode(self.lmm), self.lang)
+
+@receiver(pre_save, sender=ProductAlias)
+def check_valid_product(sender, instance, **kwargs):
+
@pmclanahan
pmclanahan Aug 15, 2013 Member

Doc string or some comments would be helpful for this one.

@pmclanahan pmclanahan commented on an outdated diff Aug 15, 2013
apps/api/views.py
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=101)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=102)
+
+ if not Product.objects.filter(name=redirect).exists():
+ return xml.error('You cannot create an alias for a product that does not exist', errno=103)
+
+ if Product.objects.filter(name=alias).exists():
+ return xml.error('You cannot create an alias with the same name as a product', errno=104)
+
+ alias_obj, created = ProductAlias.objects.get_or_create(alias=alias, defaults={'related_product': redirect})
+ if not created:
+ # assuming you want to update the redirect if it already exists.
@pmclanahan
pmclanahan Aug 15, 2013 Member

This comment was mostly for you :) A comment here is good, but the wording is odd for the context.

@brandonsavage
Contributor

I think I've addressed the chief concerns in subsequent commits, and this is ready for another pass.

@pmclanahan pmclanahan commented on the diff Aug 16, 2013
sql/incremental.sql
@@ -124,3 +124,11 @@ ALTER TABLE mirror_location_mirror_map ADD COLUMN healthy tinyint(4) NOT NULL DE
-- more space for sentry logs (bug 777516)
ALTER TABLE `sentry_log` MODIFY `reason` MEDIUMTEXT;
+
+-- create table for aliases (bug 863381)
@pmclanahan
pmclanahan Aug 16, 2013 Member

Either this or the south migration are probably superfluous. I can't imagine having both is good.

@brandonsavage
brandonsavage Aug 16, 2013 Contributor

This is a historical file, which is why i left it in place and updated it.

On Aug 16, 2013, at 4:05 PM, Paul McLanahan notifications@github.com wrote:

In sql/incremental.sql:

@@ -124,3 +124,11 @@ ALTER TABLE mirror_location_mirror_map ADD COLUMN healthy tinyint(4) NOT NULL DE

-- more space for sentry logs (bug 777516)
ALTER TABLE sentry_log MODIFY reason MEDIUMTEXT;
+
+-- create table for aliases (bug 863381)
Either this or the south migration are probably superfluous. I can't imagine having both is good.


Reply to this email directly or view it on GitHub.

@pmclanahan pmclanahan commented on an outdated diff Aug 16, 2013
apps/mirror/admin.py
@@ -1,7 +1,12 @@
from django.contrib import admin
-from mirror.models import Mirror, OS, Product, Location, ProductLanguage
+from mirror.models import Mirror, OS, Product, Location, ProductLanguage, ProductAlias
+from mirror.forms import ProductAliasAdminForm
@pmclanahan
pmclanahan Aug 16, 2013 Member

should be 2 blank lines here.

@pmclanahan pmclanahan commented on an outdated diff Aug 16, 2013
apps/mirror/admin.py
+class ProductAliasAdmin(admin.ModelAdmin):
+ list_display = ('alias', 'related_product')
+ form = ProductAliasAdminForm
+admin.site.register(ProductAlias, ProductAliasAdmin)
@pmclanahan
pmclanahan Aug 16, 2013 Member

2 blank lines here too

@pmclanahan pmclanahan commented on an outdated diff Aug 16, 2013
apps/api/views.py
+ if not Product.objects.filter(name=redirect).exists():
+ return xml.error('You cannot create an alias for a product that does not exist', errno=103)
+
+ if Product.objects.filter(name=alias).exists():
+ return xml.error('You cannot create an alias with the same name as a product', errno=104)
+
+ alias_obj, created = ProductAlias.objects.get_or_create(alias=alias, defaults={'related_product': redirect})
+ if not created:
+ # assuming you want to update the redirect if it already exists.
+ alias_obj.related_product = redirect
+ alias_obj.save()
+
+ return xml.success('Created/updated alias %s' % alias)
+
+
+
@pmclanahan
pmclanahan Aug 16, 2013 Member

Just 2 blank lines here.

@pmclanahan pmclanahan commented on an outdated diff Aug 16, 2013
apps/api/tests/test_products.py
@@ -170,3 +170,14 @@ def test_product_language_delete_all(self):
assert not myprod.languages.count(), (
'Wildcard must delete all languages.')
+
+ def test_create_update_alias(self):
@pmclanahan
pmclanahan Aug 16, 2013 Member

Should have tests for each error state as well. Some tests of the custom clean methods on the admin form would be sweet as well. Code coverage FTW! 🍰

@brandonsavage
Contributor

Pep8 fixes coming in a future revision. At this point can we merge this so QA can get on it?

@lauraxt
Contributor
lauraxt commented Aug 19, 2013

There are a bunch of non-PEP8 stuff that still needs fixing, I think

@brandonsavage
Contributor

This pull request has been updated.

@lauraxt
Contributor
lauraxt commented Aug 21, 2013

There are a few things I'd still like to see

  • pmac's request #28 (comment)
  • needs a test #28 (comment)
  • How hard is it to do the PEP8 stuff? I would have thought it would be faster to do it than to keep arguing about it :)
@fwenzel fwenzel commented on an outdated diff Aug 26, 2013
apps/api/views.py
+ E_RELATED_NAME_REQUIRED = 102
+ E_PRODUCT_DOESNT_EXIST = 103
+ E_ALIAS_PRODUCT_MATCH = 104
+
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=E_ALIAS_REQUIRED)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=E_RELATED_NAME_REQUIRED)
+
+ if not Product.objects.filter(name=redirect).exists():
+ return xml.error('You cannot create an alias for a product that does not exist', errno=E_PRODUCT_DOESNT_EXIST)
@fwenzel
fwenzel Aug 26, 2013 Member

These lines are too long. If you lint your python files, it'll tell you this too (I know I'm an offender also, but try to add line breaks where they make sense).

@fwenzel fwenzel commented on an outdated diff Aug 26, 2013
apps/api/views.py
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error('alias name is required.', errno=E_ALIAS_REQUIRED)
+
+ if not redirect:
+ return xml.error('related_product name is required.', errno=E_RELATED_NAME_REQUIRED)
+
+ if not Product.objects.filter(name=redirect).exists():
+ return xml.error('You cannot create an alias for a product that does not exist', errno=E_PRODUCT_DOESNT_EXIST)
+
+ if Product.objects.filter(name=alias).exists():
+ return xml.error('You cannot create an alias with the same name as a product', errno=E_ALIAS_PRODUCT_MATCH)
+
+ alias_obj, created = ProductAlias.objects.get_or_create(alias=alias, defaults={'related_product': redirect})
@fwenzel
fwenzel Aug 26, 2013 Member

line breaks

@fwenzel fwenzel commented on an outdated diff Aug 26, 2013
apps/mirror/admin.py
@@ -1,7 +1,12 @@
from django.contrib import admin
-from mirror.models import Mirror, OS, Product, Location, ProductLanguage
+from mirror.models import Mirror, OS, Product, Location, ProductLanguage, ProductAlias
+from mirror.forms import ProductAliasAdminForm
@fwenzel
fwenzel Aug 26, 2013 Member

imports in alphabetical order

@fwenzel fwenzel and 2 others commented on an outdated diff Aug 26, 2013
bouncer/php/index.php
@@ -58,6 +37,16 @@
);
$sdo = new SDO2($mc, $dbwrite);
+
+ // New alias code for bug 863381
+ $alias_sql = 'SELECT * FROM mirror_aliases WHERE alias = ?';
+ $alias = $sdo->get_one($alias_sql, array($product_name), SDO2::FETCH_ASSOC);
+ if($alias) {
@fwenzel
fwenzel Aug 26, 2013 Member

might be a matter of personal preference, but if is not a function :)

@brandonsavage
brandonsavage Aug 26, 2013 Contributor

Wrapping an if expression in parenthesis is required in PHP.

On Aug 26, 2013, at 1:17 PM, Fred Wenzel notifications@github.com wrote:

In bouncer/php/index.php:

@@ -58,6 +37,16 @@
);
$sdo = new SDO2($mc, $dbwrite);

  • // New alias code for bug 863381
  • $alias_sql = 'SELECT * FROM mirror_aliases WHERE alias = ?';
  • $alias = $sdo->get_one($alias_sql, array($product_name), SDO2::FETCH_ASSOC);
  • if($alias) {
    might be a matter of personal preference, but if is not a function :)


Reply to this email directly or view it on GitHub.

@pmclanahan
pmclanahan Aug 27, 2013 Member

I think he means he prefers a space between: if ($alias) {.

@fwenzel
fwenzel Aug 27, 2013 Member

Yeah, I could have explained that better. And I do realize that other people are not as strict about function calls vs. statements as I am.

@brandonsavage
Contributor

Okay, I think I've addressed the remaining concerns:

  • PEP8 compliance for all NEW code (old code that is not PEP8 compliant will be fixed in a future PR)
  • Tests that cover all API cases (success and failure)
  • Removal of the specific primary key field.

I will merge this with an r+.

@pmclanahan pmclanahan commented on an outdated diff Aug 27, 2013
apps/api/views.py
@@ -357,6 +357,57 @@ def location_delete(request):
return xml.success('Deleted location: %s' % location)
+@require_POST
+@csrf_exempt
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):
+ """Create or update an alias for a product"""
+
+ E_ALIAS_REQUIRED = 101
@pmclanahan
pmclanahan Aug 27, 2013 Member

These should be defined at the module level so they can be imported and used elsewhere (e.g. in the tests).

@pmclanahan pmclanahan commented on an outdated diff Aug 27, 2013
apps/api/views.py
@@ -357,6 +357,57 @@ def location_delete(request):
return xml.success('Deleted location: %s' % location)
+@require_POST
+@csrf_exempt
+@has_perm_or_basicauth("mirror.create_update_alias", HTTP_AUTH_REALM)
+def create_update_alias(request):
+ """Create or update an alias for a product"""
+
+ E_ALIAS_REQUIRED = 101
+ E_RELATED_NAME_REQUIRED = 102
+ E_PRODUCT_DOESNT_EXIST = 103
+ E_ALIAS_PRODUCT_MATCH = 104
+
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
@pmclanahan
pmclanahan Aug 27, 2013 Member

These values should be validated in some way. I believe django does some automatic DB sanitation of strings passed to the ORM, but I wouldn't trust only that, and it'd be a lot better to avoid sending invalid queries to the DB.

@fwenzel fwenzel commented on an outdated diff Aug 27, 2013
apps/api/tests/test_products.py
@@ -170,3 +170,68 @@ def test_product_language_delete_all(self):
assert not myprod.languages.count(), (
'Wildcard must delete all languages.')
+
+ def test_create_update_alias(self):
+
+ Product.objects.create(name='MyTestProduct')
+
+ response = self.c.post(reverse('api.views.create_update_alias'),
+ {'alias': 'my-test-alias',
+ 'related_product': 'MyTestProduct'})
+
+ self.assertTrue('Created/updated alias my-test-alias' in
@fwenzel
fwenzel Aug 27, 2013 Member

I'd say it's bad style checking for a specific return string. Instead, I'd look at the HTTP response code (that's what it's for, after all), if you got a 200 then your API call worked.

@fwenzel fwenzel commented on an outdated diff Aug 27, 2013
apps/api/tests/test_products.py
+
+ self.assertTrue('Created/updated alias my-test-alias' in
+ response.content)
+
+ def test_create_update_alias_requires_alias_name(self):
+ Product.objects.create(name='MyTestProduct')
+
+ response = self.c.post(reverse('api.views.create_update_alias'),
+ {'alias': '',
+ 'related_product': 'MyTestProduct'})
+
+ xmldoc = minidom.parseString(response.content)
+
+ msg = xmldoc.getElementsByTagName('error')
+ errno = msg[0].getAttribute('number')
+ self.assertTrue('alias name is required.' in response.content)
@fwenzel
fwenzel Aug 27, 2013 Member

same here, testing for specific error wording is not necessary if you're testing for the error number.

@fwenzel fwenzel commented on an outdated diff Aug 27, 2013
apps/api/tests/test_products.py
+ self.assertTrue('Created/updated alias my-test-alias' in
+ response.content)
+
+ def test_create_update_alias_requires_alias_name(self):
+ Product.objects.create(name='MyTestProduct')
+
+ response = self.c.post(reverse('api.views.create_update_alias'),
+ {'alias': '',
+ 'related_product': 'MyTestProduct'})
+
+ xmldoc = minidom.parseString(response.content)
+
+ msg = xmldoc.getElementsByTagName('error')
+ errno = msg[0].getAttribute('number')
+ self.assertTrue('alias name is required.' in response.content)
+ self.assertEqual(int(errno), 101,
@fwenzel
fwenzel Aug 27, 2013 Member

like pmac suggested, you want to use the error constants here.

@fwenzel fwenzel commented on an outdated diff Aug 27, 2013
apps/api/views.py
+ """Create or update an alias for a product"""
+
+ E_ALIAS_REQUIRED = 101
+ E_RELATED_NAME_REQUIRED = 102
+ E_PRODUCT_DOESNT_EXIST = 103
+ E_ALIAS_PRODUCT_MATCH = 104
+
+ xml = XMLRenderer()
+
+ alias = request.POST.get('alias', None)
+ redirect = request.POST.get('related_product', None)
+ if not alias:
+ return xml.error(
+ 'alias name is required.',
+ errno=E_ALIAS_REQUIRED
+ )
@fwenzel
fwenzel Aug 27, 2013 Member

not a mistake, but you don't have to put the closing parenthesis on a separate line.

@fwenzel fwenzel commented on the diff Aug 27, 2013
apps/mirror/admin.py
@@ -34,7 +42,8 @@ class ProductLanguageInline(admin.TabularInline):
class ProductAdmin(admin.ModelAdmin):
- list_display = ('name', 'priority', 'count', 'active', 'checknow', 'ssl_only')
+ list_display = ('name', 'priority', 'count', 'active', 'checknow',
+ 'ssl_only')
@fwenzel
fwenzel Aug 27, 2013 Member

good catch

@fwenzel fwenzel commented on the diff Aug 27, 2013
apps/mirror/forms.py
class UptakeForm(forms.Form):
p = forms.ModelMultipleChoiceField(
queryset=Product.objects.order_by('name'),
label='Products')
+
+class ProductAliasAdminForm(forms.ModelForm):
+ class Meta:
+ model = ProductAlias
+
+ def clean_alias(self):
+ if Product.objects.filter(name=self.cleaned_data['alias']).exists():
+ raise forms.ValidationError(
+ "Your alias cannot share the same name as an existing product!"
@fwenzel
fwenzel Aug 27, 2013 Member

They're not "special" in Python, but you don't want to mix single and double quotes all the time. Stick with one.

@pmclanahan
pmclanahan Aug 27, 2013 Member

Our rule is to generally stick with single quotes unless you need doubles for some reason (like there are single quotes in the string).

@fwenzel fwenzel commented on an outdated diff Aug 27, 2013
apps/mirror/models.py
@@ -78,6 +78,15 @@ class Meta:
db_table = 'mirror_products'
+class ProductAlias(models.Model):
+ """An alias for product models"""
@fwenzel
fwenzel Aug 27, 2013 Member

This is very cryptic

@pmclanahan
Member

It's an r+ from me 🍰 Thanks for fixing all the things!

Squarsh and merge at will (as long as @lauraxt agrees).

@lauraxt
Contributor
lauraxt commented Aug 27, 2013

r++ to pmac's comments.

@brandonsavage brandonsavage merged commit 226bcff into mozilla:master Aug 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment