Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Atomic service page #254

Merged
merged 1 commit into from Jun 12, 2012

Conversation

Projects
None yet
3 participants
Contributor

readevalprint commented Jun 7, 2012

removed the merge and the html api tempaltes

@jsocol jsocol and 1 other commented on an outdated diff Jun 8, 2012

apps/phonebook/views.py
@@ -66,6 +67,15 @@ def edit_profile(request):
request.FILES,
instance=profile,
)
+
+ if 'reset_api_key' in request.POST:
+ # The rest of the form is irrelevant.
+ try:
+ request.user.api_key.delete()
+ except ApiKey.DoesNotExist:
+ pass
+ return redirect(reverse('profile.edit'))
@jsocol

jsocol Jun 8, 2012

Member

As Giorgos said on the other pull req, it's probably worth doing

return redirect(urlparams(reverse('profile.edit'), fragment='services'))

So people come back to the same page.

@readevalprint

readevalprint Jun 8, 2012

Contributor

Did not know about the fragment argument. Always learning.

@jsocol jsocol commented on the diff Jun 8, 2012

apps/users/models.py
@@ -134,6 +135,13 @@ def vouch(self, vouched_by, system=True, commit=True):
# Email the user and tell them they were vouched.
self._email_now_vouched()
+ def get_api_key(self):
@jsocol

jsocol Jun 8, 2012

Member

As Giorgos said, this could be simplified to:

key, created = ApiKey.objects.get_or_create(user=self.user)
return key

@jsocol jsocol commented on an outdated diff Jun 8, 2012

apps/users/tests.py
@@ -353,6 +353,16 @@ def test_userprofile(self):
# Good to go
assert u.get_profile()
+ def test_apikey(self):
+ """Test that get_api_key() will create a key if missing."""
+ # A new user will not have a key created.
+ u = User.objects.create(username='tmp', email='tmp@domain.com')
+ p = u.get_profile()
+ from tastypie.models import ApiKey
+ # assertRaises needs a callable
@jsocol

jsocol Jun 8, 2012

Member

Rather than document assertRaises, can this comment explain what the assertion is: e.g. "First, there shouldn't be an API key."

Member

jsocol commented Jun 8, 2012

Thanks, this is much easier to review! Let's get follow-up bugs filed for the HTML API and the rest of it?

Just a couple of small comments, but I'd like to see the follow-up commit that addresses them before giving an r+.

@jsocol jsocol and 1 other commented on an outdated diff Jun 8, 2012

apps/users/tests.py
@@ -353,6 +353,16 @@ def test_userprofile(self):
# Good to go
assert u.get_profile()
+ def test_apikey(self):
+ """Test that get_api_key() will create a key if missing."""
+ # A new user will not have a key created.
+ u = User.objects.create(username='tmp', email='tmp@domain.com')
@jsocol

jsocol Jun 8, 2012

Member

If you rebase this onto master you can use the new users.tests.user modelmaker.

@readevalprint

readevalprint Jun 8, 2012

Contributor

Cool.

Member

jsocol commented Jun 8, 2012

OK, here's my request to finish this out:

  1. Fix the couple of comments. There are really only 2 or 3.
  2. Rebase on top of master, squash viciously, get it down to one commit.
  3. Let me see the result, we should be at r+ after that.
Contributor

readevalprint commented Jun 8, 2012

squashed, r?

@tallowen tallowen and 1 other commented on an outdated diff Jun 9, 2012

apps/phonebook/views.py
- form = forms.ProfileForm(
- instance=profile,
- initial=initial,
- )
+ if not request.user.username.startswith('u/'):
+ initial.update(username=request.user.username)
+
+ form = forms.ProfileForm(
@tallowen

tallowen Jun 9, 2012

Member

If this is a post request shouldn't the form returned be the one that is created here so that form errors get passed back to the view?

@jsocol

jsocol Jun 11, 2012

Member

Yeah, this should be in part of an else: block, in case it's a GET.

Member

jsocol commented Jun 11, 2012

This is basically r+ from me with the little note about the form in apps/phonebook/views.py:104 and... an apology. I wouldn't have touched the <legend>s if bug 763509 hadn't ended up requiring me to also add <h2>s to a tab content pane.

So... you should fix up that form. Tests and the view and template all look good. Then you'll probably want to rebase and deal with the conflicts I just created for you. :(

But after that, you can go ahead and land it, I think anything else is follow-up bug worthy.

Contributor

readevalprint commented Jun 12, 2012

Added else block back and squashed

readevalprint added a commit that referenced this pull request Jun 12, 2012

Merge pull request #254 from readevalprint/atomic_service_page
[fix bug 753398] service tab with API key generator

@readevalprint readevalprint merged commit 2e5fe2f into mozilla:master Jun 12, 2012

Isn't this else: the counterpart to if form.is_valid(): and not if request.method == 'POST'? Seems like the indent level is a little too far here.

Contributor

readevalprint replied Jun 12, 2012

Yup, fixed it, but am having problems with the product and cannot runt the tests locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment