Bug 1005114 - Profile includes list of completed tasks #124

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

Contributor
mjzffr commented Jun 4, 2014

User profile page displays task in progress and list of completed tasks, most recent first. The list of completed tasks is paginated. The "Welcome, user" string in the header links to the user profile. The profile page is only accessible by the user who owns it. Pagination is generalized with custom Jinja2 macro.

Also proposing a fix for how page_url helper (and associated tests) generate URL strings used for pagination.

(For previous discussion see PR #120.)

@bobsilverberg r?

mjzffr added some commits May 27, 2014
@mjzffr mjzffr Profile page displays completed tasks, most recent first d37745e
@mjzffr mjzffr Add pagination to completed tasks cf62597
@mjzffr mjzffr Add link in header to profile page. 25cd5cd
@mjzffr mjzffr Address review comments from @bobsilverberg 5d7be92
@mjzffr mjzffr Generalize pagination with custom Jinja2 macro 4ebe9ed
@mjzffr mjzffr Fix how URL with query string is formed.
dict() applied to QueryDict produces a dictionary whose values are
string representations of lists, like "[u'44']". This is because
QueryDict associates each key with a list of values even if there is
only one value. QueryDict's dict() method translates each list of
values into a string repr of *one* value from the original list. This
means data is thrown away for any keys with multiple values in the
QueryDict. This is a problem for certain form controls.
3efb06c
@mjzffr mjzffr Reuse dashboard in both front page and profile ce581ed
@mjzffr mjzffr Fix datatype of GET attribute 287e8da
Owner
mjzffr commented on 3efb06c Jun 4, 2014

(To address your previous question, @bobsilverberg.)

There are two problems in master right now:

  1. generating URLs with brackets around GET parameter values (which this commit fixes);
  2. losing some information about GET parameters that are associated with multiple values.

I propose to open a different bug for the second problem as it is significant and unrelated to the profile page.

I would also add the following test to address the issue:

    def test_repeats(self):
        """
        GET parameters with multiple values should have all their
        values preserved
        """
        request = Mock(GET=QueryDict('foo=bar&baz=5&foo=ok'))
        url = urlparse.urlsplit(page_url(request, 4))
        args = urlparse.parse_qs(url.query)
        eq_(args, {'foo': ['bar', 'ok'], 'baz': ['5'], 'page': ['4']})

(Copying comments from my broken pull-request to continue discussion here.)

FWIW, see https://github.com/mozilla/funfactory/blob/master/funfactory/helpers.py#L48 and note that "New query params will be appended to exising parameters, except duplicate names, which will be replaced." Since urlparams('', QueryDict('foo=bar&baz=5&foo=ok')) yields a URL that ends with "foo=[u'bar',+u'ok']&baz=[u'5']", I think urlparams is expecting a dictionary where each key has a single value (not a list of values).

"@Osmose can we get your thoughts on this, as you wrote the original code? Is the above change OK/needed, or do we need to keep the code as is and find another way of dealing with the fact that we are having issues with the URL that is generated when doing pagination and filtering at the same time?"

request.GET.dict() seems fine to me.

If you're really concerned about multiple values for a key (which, if we're not currently using, seems fine to ignore until it becomes an issue), then I think the real fix is a patch to funfactory that fixes urlparams to work with QueryDicts, or to just not use urlparams and find another way of building the query string.

Thanks @Osmose. I agree that if we don't currently need support for duplicate url params then we don't really need to worry about it for now. Hopefully if we do in the future my tired old brain will remember this conversation. :)

@mjzffr I'll do another review of the current PR.

Owner
mjzffr commented on 287e8da Jun 4, 2014

Django's QueryDict does not behave quite like a dictionary, as described in my previous commit, so these tests were missing the incorrect behaviour we observed with pagination links for a filtered list. (The incorrect behaviour -- generating URLs with brackets around GET parameter values -- actually exists in master right now.)

>>> from django.http import QueryDict
>>> q = QueryDict('a=1&b=2&c=3')
>>> q
<QueryDict: {u'a': [u'1'], u'c': [u'3'], u'b': [u'2']}>
>>> dict(q)
{u'a': [u'1'], u'c': [u'3'], u'b': [u'2']}
>>> q.dict()
{u'a': u'1', u'c': u'3', u'b': u'2'}
>>> r = QueryDict('a=1&b=2&c=3&a=4')
>>> dict(r)
{u'a': [u'1', u'4'], u'c': [u'3'], u'b': [u'2']}
>>> r.dict()
{u'a': u'4', u'c': u'3', u'b': u'2'}
Collaborator

@mjzffr This replaces PR #120, correct? If so you can close that other PR.

Collaborator

LGTM @mjzffr.

@Osmose Do you mind giving it a once over before I merge it?

@Osmose Osmose commented on the diff Jun 4, 2014
oneanddone/tasks/models.py
@@ -145,6 +145,9 @@ class TaskAttempt(CreatedModifiedModel):
def __unicode__(self):
return u'{user} attempt [{task}]'.format(user=self.user, task=self.task)
+ class Meta(CreatedModifiedModel.Meta):
Osmose
Osmose Jun 4, 2014 Owner

Woah, never seen this before, coooooooool

@Osmose Osmose commented on an outdated diff Jun 4, 2014
oneanddone/users/models.py
@@ -37,7 +37,6 @@ def user_attempts_finished_count(self):
return self.taskattempt_set.filter(state=TaskAttempt.FINISHED).count()
User.add_to_class('attempts_finished_count', user_attempts_finished_count)
-
Osmose
Osmose Jun 4, 2014 Owner

Two spaces between declarations at the module-level please. :D

@Osmose Osmose commented on an outdated diff Jun 4, 2014
oneanddone/users/templates/users/profile/detail.html
@@ -6,30 +6,24 @@
{% set title = _('User profile for {username}')|f(username=user.profile.name) %}
{% block content %}
+<h1>{% trans name=user.profile.name %}
Osmose
Osmose Jun 4, 2014 Owner

{{ _('{name}'s Profile')|fe(name=user.profile.name) }} is a more concise version of this that looks better on one line.

fe being a lovely filter provided by playdoh: http://playdoh.readthedocs.org/en/latest/bestpractices.html#safe-considered-harmful

@Osmose Osmose commented on an outdated diff Jun 4, 2014
oneanddone/users/templates/users/profile/detail.html
- <section class="task-status">
- <h3>{{ _('Tasks completed') }}</h3>
+ <h3>{{ ngettext('{count} task', '{count} tasks', attempts_finished)|f(count=user.attempts_finished_count) }} {{ _('completed') }}</h3>
Osmose
Osmose Jun 4, 2014 Owner

"completed" should be added to the strings being passed to ngettext, it can't be added on to the end for some locales.

@Osmose Osmose commented on an outdated diff Jun 4, 2014
oneanddone/users/views.py
@@ -57,6 +59,29 @@ class UpdateProfileView(UserProfileRequiredMixin, generic.UpdateView):
def get_object(self):
return self.request.user.profile
+class ProfileDetailsView(UserProfileRequiredMixin, generic.DetailView):
+ model = UserProfile
+ template_name = 'users/profile/detail.html'
+
+ def get_object(self):
+ return self.request.user.profile
+
+ def get_context_data(self, **kwargs):
+ all_attempts_finished = self.request.user.taskattempt_set.filter(state=TaskAttempt.FINISHED)
+ #add pagination preferences to UserProfile?
Osmose
Osmose Jun 4, 2014 Owner

Probably add a TODO: to the front of this in case you're considering doing this, that way people might find it later by searching. Or leave it out and just file an issue for the idea.

@Osmose Osmose and 1 other commented on an outdated diff Jun 4, 2014
oneanddone/users/views.py
@@ -57,6 +59,29 @@ class UpdateProfileView(UserProfileRequiredMixin, generic.UpdateView):
def get_object(self):
return self.request.user.profile
+class ProfileDetailsView(UserProfileRequiredMixin, generic.DetailView):
+ model = UserProfile
+ template_name = 'users/profile/detail.html'
+
+ def get_object(self):
+ return self.request.user.profile
+
+ def get_context_data(self, **kwargs):
+ all_attempts_finished = self.request.user.taskattempt_set.filter(state=TaskAttempt.FINISHED)
+ #add pagination preferences to UserProfile?
+ num_items_per_page = 20
Osmose
Osmose Jun 4, 2014 Owner

This is used once, just pass a 20 into the Paginator directly.

bobsilverberg
bobsilverberg Jun 4, 2014 Collaborator

This is my fault. I don't like hardcoded values, but I guess all we've done here is hardcode the value into a variable as opposed to the actual method.

Osmose
Osmose Jun 4, 2014 Owner

Single hardcoded values aren't nearly as bad as hardcoded values used in multiple places. It's easier to replace if it's just used once.

@Osmose Osmose commented on an outdated diff Jun 4, 2014
oneanddone/users/views.py
+ def get_object(self):
+ return self.request.user.profile
+
+ def get_context_data(self, **kwargs):
+ all_attempts_finished = self.request.user.taskattempt_set.filter(state=TaskAttempt.FINISHED)
+ #add pagination preferences to UserProfile?
+ num_items_per_page = 20
+ paginator = Paginator(all_attempts_finished, num_items_per_page)
+ page = self.request.GET.get('page', 1)
+ try:
+ attempts_finished = paginator.page(page)
+ except PageNotAnInteger:
+ attempts_finished = paginator.page(1)
+ except EmptyPage:
+ attempts_finished = paginator.page(paginator.num_pages)
+ context = super(ProfileDetailsView, self).get_context_data(**kwargs)
Osmose
Osmose Jun 4, 2014 Owner

Could use a newline here (and maybe a few above in this function) to separate sections of the function and make it a little more readable.

Owner
Osmose commented Jun 4, 2014

r+wc, most of my comments are just stylistic, the l10n one is kinda important. I didn't run this locally, but I assume @bobsilverberg did and saw that it works well, so I'm not worried.

Nice work! :D

Collaborator

Great work @mjzffr. Time to merge!

Collaborator

I squashed the commits locally and pushed so final commit is 52e4d7a

@mjzffr mjzffr deleted the mjzffr:profiledetail branch Jun 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment