switch reviewers app history to use comm api (bug 920746) #1607

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

ngokevin commented Jan 9, 2014

Context:

  • The Reviewer Tools review pages for individual apps have a review history table that details previous review actions, reviewer comments, and attachments by version.
  • When reviewing/commenting/attaching a file, an ActivityLog object is created.
  • We have a command to migrate review history/comments/attachments from ActivityLog to Commbadge objects (which feature an API)

Solution:

  • We change the review history table to query the Commbadge Threads/Notes/Attachments APIs via JS, trying to keep everything looking the same.
  • We keep this under a waffle, so when the waffle is off, we use the template that queries ActivityLog objects.
  • If we ever flip the switch, we need to make sure all the objects are migrated. We can run the migration command, but then any incoming reviewer actions will also need to be migrated. Thus, we must be creating Commbadge Threads/Notes/Attachments alongside ActivityLog objects in the reviewer utils

Notes:

  • It's a big patch, but since commbadge is flipped on on -dev, it may be easier to do everything in one fell swoop.
  • Depends on #1629 for the attachment creation utility.

Screenshot:

screen shot 2014-01-08 at 6 39 35 pm

Contributor

ngokevin commented Jan 14, 2014

@cvan r?

+ var threadIdPlaceholder = $itemHistory.data('thread-id-placeholder');
+ var noteTypes = $itemHistory.data('note-types');
+
+ var noteTemplate = _.template($('#commbadge-note').html());
@cvan

cvan Jan 14, 2014

Member

you're not passing it any context?

@ngokevin

ngokevin Jan 14, 2014

Contributor

I pass it below, just generating a template factory here.

+
+ // Get all of the app's threads.
+ $.get(_userArg(commThreadUrl), function(data) {
+ $threads = $(data.objects);
@cvan

cvan Jan 14, 2014

Member

why parse it as a jQuery object?

+
+ // Show "this version has not been reviewed" for table w/ no results.
+ var versionIds = [];
+ $threads.each(function(i, thread) {
@cvan

cvan Jan 14, 2014

Member

just use _.map

+ $table.append(noteTemplate({
+ attachments: note.attachments,
+ body: note.body,
+ metadata: format(gettext('By {0} on {1}'),
@cvan

cvan Jan 14, 2014

Member

add l10n comment

media/js/devreg/utils.js
+}
+
+
+function getVars(qs, excl_undefined) {
@cvan

cvan Jan 14, 2014

Member

you surrre we don't already have this somewhere?

@ngokevin

ngokevin Jan 14, 2014

Contributor

Only surre, I'll check to be surrre.

@@ -181,6 +181,7 @@
),
'mkt/reviewers': (
'js/lib/highcharts.src.js', # Used by editors.js
+ 'js/lib/moment-with-langs.min.js', # JS date lib.
mkt/constants/comm.py
@@ -1,3 +1,6 @@
+from tower import ugettext as _
@cvan

cvan Jan 14, 2014

Member

this needs to be _lazy!

+ <div class="log-group log-attachments">
+ <strong>{{ _('Attachments') }}</strong>
+ <ul>
+ <% _.each(attachments, function(attachment){ %>
@cvan

cvan Jan 14, 2014

Member

space before the {

+ <%= attachment.display_name %>
+ </a>
+ </li>
+ <% }); %>
@cvan

cvan Jan 14, 2014

Member

you can omit the semicolons. nobody's watching in underscore templates.

+<script type="text/template" id="no-notes">
+ <tr>
+ <td class="no-activity">
+ {{ _('This version has not been reviewed.') }}
@cvan

cvan Jan 14, 2014

Member

no period

+<div id="history" class="island alpha c">
+ <div id="review-files-header">
+ <h3>
+ {{ _('App History') }}
@cvan

cvan Jan 14, 2014

Member

put on same line

+ </div>
+ <table id="review-files" class="item-history">
+ {% for i in range(pager.object_list.count(), 0, -1) %}
+ {% set version = pager.object_list[i-1] %}
@cvan

cvan Jan 14, 2014

Member

indent

@cvan

cvan Jan 14, 2014

Member

spaces: i - 1

+ <td class="files">
+ {% set version_files = version.all_files %}
+ {% if version_files %}
+ <div><strong>{{ _('Files in this version:') }}</strong></div>
@cvan

cvan Jan 14, 2014

Member

feeel like this should be a heading

+ </tr>
+ {% endif %}
+ {% if version.approvalnotes %}
+ <tr>
@cvan

cvan Jan 14, 2014

Member

indent

+ {% if not version.releasenotes and not version.approvalnotes and not records %}
+ <tr>
+ <td class="no-activity">
+ {{ _('This version has not been reviewed.') }}
@cvan

cvan Jan 14, 2014

Member

no period

@@ -0,0 +1,109 @@
+<div id="history" class="island alpha c">
@cvan

cvan Jan 14, 2014

Member

feel like this a copy+paste of the reviewer tools

@ngokevin

ngokevin Jan 14, 2014

Contributor

Yeah, abstracted it out into an include, but under a waffle.

Member

cvan commented Feb 4, 2014

@ngoke where we @ w/ this?

Contributor

ngokevin commented Feb 4, 2014

Have to add some attachments logic. I'm not sure if Commbadge is on the backburner or not. I'm currently working on moving the Themes Reviewer Tools that's blocking Guillotine.

Contributor

ngokevin commented Feb 19, 2014

Breaking up the PR for easier review.

@ngokevin ngokevin closed this Feb 19, 2014

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