Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete #245

tessie · 2015-01-12T09:59:10Z

No description provided.

tessie · 2015-01-12T10:04:55Z

oneanddone/tasks/models.py

@@ -34,6 +34,7 @@ def __unicode__(self):
 class Feedback(CachedModel, CreatedModifiedModel):
    attempt = models.OneToOneField('TaskAttempt')
    text = models.TextField()
+    completion_time_in_minutes = models.IntegerField(max_length=5,blank=True,null=True)


Added a new field.to save the time taken to complete.

tessie · 2015-01-12T18:24:16Z

@bobsilverberg .please review

bobsilverberg · 2015-01-13T15:08:24Z

oneanddone/base/static/css/one-and-done.less

@@ -166,7 +166,7 @@ footer {
 }
 .feedback-form {
    width: 95%;
-    text-align: right;
+    text-align: left;


While this makes the new text input appear properly aligned, it also causes the "No Thanks" and "Submit Feedback" buttons to now appear in the lower left of the form, as opposed to the lower right, which is where they are on the current site, and where they need to stay, so this is not correct.

I would suggest that any time you are changing existing css (such as here, where you are changing an existing property from right to left), you need to be very careful and review the impact of the change on the site. This is something you could have noticed if you would have compared the feedback form currently on the site and the new feedback form produced with this change.

bobsilverberg · 2015-01-13T15:17:49Z

This is great work! Thanks @tessie. I have added a few comments to address, but overall this is very good. 👍

One other thing I would suggest adding would be to update the email that gets sent out to the task owner when feedback is submitted to include this new field.

Let me know when this is ready for another review by me.

tessie · 2015-01-18T14:18:59Z

oneanddone/base/static/css/one-and-done.less

@@ -176,6 +176,9 @@ footer {
    .no-feedback {
        margin-right: 1rem;
    }
+    .right {


Now the feedback and no thanks buttons are moved to right

tessie · 2015-01-18T14:22:57Z

@bobsilverberg .Ready to review
Changed the model name
Added validator which now removed the exception for me

bobsilverberg · 2015-01-19T20:37:01Z

...tasks/migrations/0023_auto__add_field_feedback_how_many_minutes_did_you_spend_on_the_task.py

@@ -0,0 +1,183 @@
+# -*- coding: utf-8 -*-


This one needs to be prefixed with 0024 as I mentioned in my last comment. The PR that I just merged added migrations for 0022 and 0023.

bobsilverberg · 2015-01-19T20:53:15Z

Good work, @tessie. I added a couple of comments to be addressed, but other than that it looks really good.

tessie · 2015-01-20T19:56:28Z

@bobsilverberg .time spent in minutes

bobsilverberg · 2015-01-23T22:09:19Z

This is perfect now, thanks @tessie!

I realized that there is one other place where we should add this: to the Task Activity screen. I suggest adding a field below the date that the task was finished/abandoned/etc, indicating the amount of time spent on the task. Please only display this field if there is a value for that field, as many records will not have a value.

Please squash the commits now, as I know that everything so far in this PR is good, and then add a new commit with the changes for the Task Activity screen.

Thanks!

tessie · 2015-01-26T19:16:21Z

@bobsilverberg .ready for review

bobsilverberg · 2015-01-26T19:31:36Z

oneanddone/tasks/templates/tasks/activity_listing.html

@@ -24,6 +24,12 @@
          <div>{{ _("Team: {team}")|fe(team=attempt.task.team.name) }}</div>
          <div>{{ _('Date {state}: {modified}')|fe(state=attempt.get_state_display(),
            modified=attempt.modified.strftime('%Y-%m-%d: %I:%M:%S %p').lower()) }}</div>
+          <div>


I suggest moving the <div> inside the {% if so that if we do not have a time_spent_in_minutes we do not even output the <div> to the page.

bobsilverberg · 2015-01-26T19:36:33Z

Great work @tessie. I have added a few comments for you to address.

tessie · 2015-01-26T21:09:24Z

@bobsilverberg .Thanks updated the pr

bobsilverberg · 2015-01-27T14:40:36Z

oneanddone/tasks/templates/tasks/activity_listing.html

@@ -24,6 +24,15 @@
          <div>{{ _("Team: {team}")|fe(team=attempt.task.team.name) }}</div>
          <div>{{ _('Date {state}: {modified}')|fe(state=attempt.get_state_display(),
            modified=attempt.modified.strftime('%Y-%m-%d: %I:%M:%S %p').lower()) }}</div>
+          {% if attempt.has_feedback %}


I believe that the code you wrote above for attempt.time_spent_in_minutes will return None if attempt.has_feedback is False, so I think you could just use {% if attempt.time_spent_in_minutes %} and not even bother checking if attempt.has_feedback.

bobsilverberg · 2015-01-27T14:44:12Z

This looks good, @tessie. I added one comment to address.

Also, I think you misunderstood what I meant about squashing. Because this PR has gone on for a long time and there are lots of old commits, I wanted you to squash all the old commits, but then to start adding new commits after that. I didn't want you to just start squashing every time you added a commit. I want to be able to see the exact changes that were just made, so if you squash everything I cannot see that. I did explicitly say above "Please squash the commits now, as I know that everything so far in this PR is good, and then add a new commit with the changes for the Task Activity screen", but it seems like you misunderstood.

Does this make sense now?

tessie · 2015-01-27T15:28:53Z

@bobsilverberg .thanks for review.updated the pr

bobsilverberg · 2015-01-27T15:51:02Z

This is great, thanks @tessie!

I notice that there are now some merge conflicts because of other PRs that have been merged. Can you squash the commits and try to resolve the merge conflicts? If you have trouble doing that let me know and I can help.

…for a task to complete

tessie · 2015-01-28T16:04:31Z

@bobsilverberg .hopefully now it is fixed

bobsilverberg · 2015-02-02T19:45:56Z

Perfect! Thanks @tessie. Merging now!

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete

tessie reviewed Jan 12, 2015
View reviewed changes

bobsilverberg reviewed Jan 13, 2015
View reviewed changes

tessie force-pushed the featureBug1014435 branch 2 times, most recently from c3571b2 to 2ba1568 Compare January 18, 2015 14:17

tessie reviewed Jan 18, 2015
View reviewed changes

tessie force-pushed the featureBug1014435 branch from 2ba1568 to 2959963 Compare January 18, 2015 14:57

bobsilverberg reviewed Jan 19, 2015
View reviewed changes

tessie force-pushed the featureBug1014435 branch 2 times, most recently from dfb0389 to 63ccb51 Compare January 24, 2015 18:29

bobsilverberg reviewed Jan 26, 2015
View reviewed changes

tessie force-pushed the featureBug1014435 branch from 63ccb51 to d9eb780 Compare January 26, 2015 21:07

bobsilverberg reviewed Jan 27, 2015
View reviewed changes

tessie force-pushed the featureBug1014435 branch from fcd561a to 97f0376 Compare January 27, 2015 15:27

tessie force-pushed the featureBug1014435 branch 6 times, most recently from 96a4364 to fd9f946 Compare January 28, 2015 15:26

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user …

17455bc

…for a task to complete

tessie force-pushed the featureBug1014435 branch from fd9f946 to 17455bc Compare January 28, 2015 15:35

bobsilverberg added a commit that referenced this pull request Feb 2, 2015

Merge pull request #245 from tessie/featureBug1014435

39a0084

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete

bobsilverberg merged commit 39a0084 into mozilla:master Feb 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete #245

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete #245

tessie commented Jan 12, 2015

tessie Jan 12, 2015

tessie commented Jan 12, 2015

bobsilverberg Jan 13, 2015

bobsilverberg commented Jan 13, 2015

tessie Jan 18, 2015

tessie commented Jan 18, 2015

bobsilverberg Jan 19, 2015

bobsilverberg commented Jan 19, 2015

tessie commented Jan 20, 2015

bobsilverberg commented Jan 23, 2015

tessie commented Jan 26, 2015

bobsilverberg Jan 26, 2015

bobsilverberg commented Jan 26, 2015

tessie commented Jan 26, 2015

bobsilverberg Jan 27, 2015

bobsilverberg commented Jan 27, 2015

tessie commented Jan 27, 2015

bobsilverberg commented Jan 27, 2015

tessie commented Jan 28, 2015

bobsilverberg commented Feb 2, 2015

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete #245

Fix Bug 1014435 - [admin] Show the duration (time taken) by the user for a task to complete #245

Conversation

tessie commented Jan 12, 2015

tessie Jan 12, 2015

Choose a reason for hiding this comment

tessie commented Jan 12, 2015

bobsilverberg Jan 13, 2015

Choose a reason for hiding this comment

bobsilverberg commented Jan 13, 2015

tessie Jan 18, 2015

Choose a reason for hiding this comment

tessie commented Jan 18, 2015

bobsilverberg Jan 19, 2015

Choose a reason for hiding this comment

bobsilverberg commented Jan 19, 2015

tessie commented Jan 20, 2015

bobsilverberg commented Jan 23, 2015

tessie commented Jan 26, 2015

bobsilverberg Jan 26, 2015

Choose a reason for hiding this comment

bobsilverberg commented Jan 26, 2015

tessie commented Jan 26, 2015

bobsilverberg Jan 27, 2015

Choose a reason for hiding this comment

bobsilverberg commented Jan 27, 2015

tessie commented Jan 27, 2015

bobsilverberg commented Jan 27, 2015

tessie commented Jan 28, 2015

bobsilverberg commented Feb 2, 2015