Skip to content
This repository was archived by the owner on Jan 29, 2025. It is now read-only.

Recipe signing#190

Merged
mythmon merged 8 commits intomozilla:signingfrom
mythmon:recipe-signing
Jul 14, 2016
Merged

Recipe signing#190
mythmon merged 8 commits intomozilla:signingfrom
mythmon:recipe-signing

Conversation

@mythmon
Copy link
Copy Markdown
Contributor

@mythmon mythmon commented Jul 7, 2016

Retargeting #180 at a branch.

@Osmose r?

@mythmon mythmon mentioned this pull request Jul 7, 2016

class SignedRecipeSerializer(serializers.ModelSerializer):
signature = SignatureSerializer()
recipe = serializers.SerializerMethodField()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs suggest you can just use RecipeSerializer as the field and get the same effect: http://www.django-rest-framework.org/api-guide/relations/#nested-relationships

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that first. The problem is that this serializer is serializing a Recipe, and needs to pass itself to the nested serializer. If this line were just recipe = RecipeSerializer(), it would try and look for self.recipe, which doesn't exist. I also couldn't figure out anything I could supply as the source for the serializer to make it point to itself either.

@Osmose
Copy link
Copy Markdown
Contributor

Osmose commented Jul 8, 2016

Just to check my understanding: Signatures are meant to be deleted when they're invalidated, right?

Also, this PR doesn't include creating signatures outside of the management command. That's waiting for peer signing, right? And will block this branch from getting merged?

@@ -0,0 +1,27 @@
from datetime import datetime
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing the factories?!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure there were working just right. So I tested it.

@Osmose
Copy link
Copy Markdown
Contributor

Osmose commented Jul 8, 2016

Missing tests for skip_last_updated and for the management command.

Alright, I think that covers my first pass. Nice work thusfar!

@mythmon mythmon assigned mythmon and unassigned Osmose Jul 13, 2016
Comment thread normandy/recipes/tests/test_models.py Outdated
signatures = list(Recipe.objects.all().values_list('signature__signature', flat=True))
assert signatures == ['fake signature', 'fake signature']

def test_update_signatures_increments_revision_id(self, mocker):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked offline about removing this test, let's not keep it.

@Osmose
Copy link
Copy Markdown
Contributor

Osmose commented Jul 14, 2016

r+wc if the tests pass after you remove that failure

@mythmon mythmon merged commit 12bdbf1 into mozilla:signing Jul 14, 2016
@mythmon mythmon deleted the recipe-signing branch July 14, 2016 23:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants