mark rejected apps as STATUS_REJECTED and allow developers to resubmit (bug 737727)

2012-04-04 10:37:52 -07:00 · 2012-04-04 10:37:52 -07:00 · 446b2a7b87
--- a/apps/constants/base.py
+++ b/apps/constants/base.py
@ -18,6 +18,7 @@ STATUS_LITE = 8
 STATUS_LITE_AND_NOMINATED = 9
 STATUS_PURGATORY = 10  # A temporary home; bug 614686
 STATUS_DELETED = 11
+STATUS_REJECTED = 12  # This applies only to apps (for now)

 STATUS_CHOICES = {
    STATUS_NULL: _(u'Incomplete'),
@ -31,8 +32,8 @@ STATUS_CHOICES = {
    STATUS_LITE: _(u'Preliminarily Reviewed'),
    STATUS_LITE_AND_NOMINATED:
        _(u'Preliminarily Reviewed and Awaiting Full Review'),
-    STATUS_PURGATORY:
-        _(u'Pending a review choice'),
+    STATUS_PURGATORY: _(u'Pending a review choice'),
+    STATUS_REJECTED: _(u'Rejected')
 }

 REVIEWED_STATUSES = (STATUS_LITE, STATUS_LITE_AND_NOMINATED, STATUS_PUBLIC)
--- a/apps/devhub/helpers.py
+++ b/apps/devhub/helpers.py
@ -172,6 +172,8 @@ def status_class(addon):
        amo.STATUS_LITE: 'lite',
        amo.STATUS_LITE_AND_NOMINATED: 'lite-nom',
        amo.STATUS_PURGATORY: 'purgatory',
+        amo.STATUS_DELETED: 'deleted',
+        amo.STATUS_REJECTED: 'rejected',
    }
    if addon.disabled_by_user and addon.status != amo.STATUS_DISABLED:
        cls = 'disabled'
--- a/apps/files/utils.py
+++ b/apps/files/utils.py
@ -214,7 +214,7 @@ class WebAppParser(object):
                'type': amo.ADDON_WEBAPP,
                'name': {default_locale: data['name']},
                'summary': self.trans_all_locales(localized_descr),
-                'version': '1.0',
+                'version': data.get('version'),
                'default_locale': default_locale}

    def trans_locale(self, locale):
--- a/media/css/devreg/status.less
+++ b/media/css/devreg/status.less
@ -1,5 +1,31 @@
@import 'lib';

+#rejection {
+    blockquote {
+        margin: 10px 0 0;
+    }
+    + p {
+        margin-top: 20px;
+    }
+}
+
+#version-status {
+    h3 {
+        color: @medium-gray;
+        font: normal 13px @sans-stack;
+        time {
+            color: @note-gray;
+            font-weight: normal;
+            &:after {
+                content: ":";
+            }
+        }
+    }
+    label {
+        margin-top: 5px;
+    }
+}
+
 .version-status-actions {
    border-top: 1px dotted @taupe;
    margin-top: 10px;
@ -39,6 +65,13 @@
    color: #329902;
 }

+.status-rejected {
+    display: inline;
+    b {
+        color: #800;
+    }
+}
+
 .delete-button:hover,
 .delete-button {
    background: #c22;
--- a/media/css/devreg/typography.less
+++ b/media/css/devreg/typography.less
@ -56,8 +56,10 @@ h4 {
 }

 blockquote {
-    margin: 2em 1em;
+    border-left: 1px solid @light-gray;
    font-family: @sans-stack;
+    margin: 2em 1em;
+    padding-left: 1em;
 }

 h5 {
--- a/media/css/mkt/reviewers.less
+++ b/media/css/mkt/reviewers.less
@ -50,6 +50,7 @@ header {
    }
    #actions {
        float: right;
+        margin-top: 15px;
        li {
            display: inline-block;
            margin-left: 10px;
@ -72,12 +73,9 @@ header {
 #addon {
    float: left;
    width: 75%;
-    #review-files-header,
-    .results {
-        display: none;
-    }
    p {
        color: @medium-gray;
+        line-height: 15px;
    }
 }

@ -522,14 +520,14 @@ table.data-grid tr.comments td {
 }

 div.editor-waiting {
-    border: 1px solid #999999;
-    border-radius: 5px 5px 5px 5px;
+    border: 1px solid @note-gray;
+    .border-radius(5px);
    overflow: hidden;
 }

 div.editor-stats-dark {
    background-color: #F3F3F3;
-    border-top: 1px dotted #CCCCCC;
+    border-top: 1px dotted @medium-gray;
    display: block;
    padding: 5px 10px 10px;
 }
@ -541,7 +539,7 @@ div.editor-stats-table > div.editor-stats-dark {
 /* Moderated reviews css */

 #reviews-flagged .review-flagged {
-    border-top: 1px dotted #CCCCCC;
+    border-top: 1px dotted @medium-gray;
    padding: 1em;
    margin: 0 0 1em 1em;
 }
@ -556,7 +554,7 @@ div.editor-stats-table > div.editor-stats-dark {

 #reviews-flagged .reviews-flagged-reasons {
    background-color: #FFE4E4;
-    border-radius: 10px;
+    .border-radius(10px);
    margin: 0;
    padding: 0 10px;
    clear: both;
@ -579,7 +577,7 @@ div.editor-stats-table > div.editor-stats-dark {
    float: right;
    padding-left: 10px;
    margin-left: 10px;
-    border-left: 1px dashed #ccc;
+    border-left: 1px dashed @medium-gray;
    width: 200px;
 }

@ -703,20 +701,43 @@ div.editor-stats-table > div.editor-stats-dark {
    top: 10px;
 }

-#review-files {
-    margin: 0;
+#history {
+    .border-box;
+    .border-radius(5px);
+    padding: 0;
    width: 100%;
+    #review-files-header {
+        padding: 0 15px;
+    }
+    h3 span {
+        color: @medium-gray;
+        padding: 0 5px;
+    }
+    table {
+        width: 100%;
+    }
+    blockquote {
+        color: @medium-gray;
+        margin: 10px 0 0;
+    }
 }
-#review-files .files {
+
+#review-files {
+    width: 100%;
+    .listing-header > th {
+        background: @faded-blue;
+        padding: 5px 15px;
+    }
+    .listing-header,
+    .listing-body {
+         border-top: 1px solid #A5BFCE;
+    }
+}
+#review-files td {
+    .border-radius(0 0 5px 5px);
    background-color: #FAFAFA;
-    border-right: 1px dotted #DDDDDD;
-    border-top: 0 none;
-    width: 255px;
    padding: 15px;
 }
-#review-files-header h3 {
-    float: left;
-}
 #review-files-header h3 span {
    font-size: 0.7em;
 }
@ -736,16 +757,21 @@ div.editor-stats-table > div.editor-stats-dark {
    top: -20px;
    width: auto;
 }
-#review-files table.activity {
-    margin: 0 10px;
-    width: 100%;
-}
 #review-files table.activity th {
  width: 25%;
  padding-right: 1em;
 }
 #review-files table.activity tr {
    border-top: 1px dotted #ccc;
+    td {
+        padding: 15px 0;
+    }
+    &:first-child td {
+        padding-top: 0;
+    }
+    &:last-child td {
+        padding-bottom: 0;
+    }
 }
 #review-files table.activity tr:first-child {
    border-top: 0 none;
@ -755,10 +781,6 @@ div.editor-stats-table > div.editor-stats-dark {
    color: #999;
 }
 #review-files tr.listing-header:first-child {
-    border-top: none;
-}
-#review-files tr.listing-header {
-     border-top: 1px solid #A5BFCE;
 }
 #review-files tr.listing-header:hover {
    cursor: pointer;
@ -812,7 +834,7 @@ div.editor-stats-table > div.editor-stats-dark {
    .box-shadow(0 -2px 0 @faded-blue inset,
                0 0 1px rgba(0, 0, 0, 0.1));
    border: 1px solid @border-blue;
-    margin-top: 20px;
+    margin-top: 15px;
 }

 .review-actions #review-actions-form {
@ -937,11 +959,6 @@ div.editor-stats-table > div.editor-stats-dark {
    font-style: italic;
 }

-.history-notes {
-    max-width: 370px;
-    overflow: auto;
-}
-
 .review-paging {
    border-bottom: 1px dotted #ccc;
    margin-bottom: 2em;
--- a/mkt/developers/forms.py
+++ b/mkt/developers/forms.py
@ -31,7 +31,8 @@ from mkt.webapps.models import Webapp
 from . import tasks
 from lib.video import tasks as vtasks

-paypal_log = commonware.log.getLogger('z.paypal')
+log = commonware.log.getLogger('mkt.developers')
+paypal_log = commonware.log.getLogger('mkt.paypal')


 class AuthorForm(happyforms.ModelForm):
@ -561,3 +562,27 @@ class CurrencyForm(happyforms.Form):
                                .distinct())
        self.fields['currencies'].choices = [(k, amo.PAYPAL_CURRENCIES[k])
                                              for k in choices]
+
+
+class AppAppealForm(happyforms.Form):
+    """
+    If a developer's app is rejected he can make changes and request
+    another review.
+    """
+
+    release_notes = forms.CharField(
+        label=_(u'Your comments'),
+        required=False, widget=forms.Textarea(attrs={'rows': 2}))
+
+    def __init__(self, *args, **kw):
+        self.product = kw.pop('product', None)
+        super(AppAppealForm, self).__init__(*args, **kw)
+
+    def save(self):
+        v = self.product.versions.order_by('-created')[0]
+        v.releasenotes = self.cleaned_data['release_notes']
+        v.save()
+        amo.log(amo.LOG.EDIT_VERSION, v.addon, v)
+        # Mark app as pending again.
+        self.product.mark_done()
+        return v
--- a/mkt/developers/templates/developers/apps/status.html
+++ b/mkt/developers/templates/developers/apps/status.html
@ -15,7 +15,7 @@
  <section id="edit-addon" class="primary devhub-form manage">
    <h2>{{ _('Current Status') }}</h2>
    <div class="island" id="version-status">
-      <p>
+      <p class="status">
        {% if addon.disabled_by_user and addon.status != amo.STATUS_DISABLED %}
          {{ status(_('You have <b>disabled</b> this app.')|safe) }}
          {{ loc("Your app's listing is disabled and will not appear in the
@ -27,17 +27,47 @@
          </a>
        {% elif addon.status == amo.STATUS_PENDING %}
          {{ status(_('This app is <b>awaiting review</b>.')|safe) }}
-          {{ loc('You will receive an email when the review is complete.') }}
+          {{ _('You will receive an email when the review is complete.') }}
        {% elif addon.status == amo.STATUS_PUBLIC %}
          {{ status(_('This app is <b>public</b>.')|safe) }}
        {% elif addon.status == amo.STATUS_DISABLED %}
          {{ status(_('This app has been <b>disabled by Mozilla</b>.')|safe) }}
          {{ loc("Your app was disabled by a site administrator. If you have any
                  questions, please email app-reviews@mozilla.org.") }}
+        {% elif addon.status == amo.STATUS_REJECTED %}
+          {{ status(_('This app has been <b>rejected</b> by a Mozilla Marketplace reviewer.')|safe) }}
        {% endif %}
        {% if not (addon.is_disabled or addon.is_incomplete()) %}
-          <a href="https://developer.mozilla.org/en/Apps/Marketplace_Review">
-            {{ _('Learn more') }}</a>
+          <a href="https://developer.mozilla.org/en/Apps/Marketplace_Review"
+             target="_blank">{{ _('Learn more') }}</a>
+        {% endif %}
+        {% if addon.status == amo.STATUS_REJECTED %}
+          <form method="post">
+            {{ csrf() }}
+            {% if rejection %}
+              <section class="island swagger" id="rejection">
+                <h3>
+                  {% trans reviewer=rejection.user.name %}
+                    Reviewed by <b>{{ reviewer }}</b>
+                  {% endtrans %}
+                  <time datetime="{{ rejection.created|isotime }}"
+                        title="{{ rejection.created|babel_datetime }}">
+                    ({{ rejection.created|timesince }})</time>
+                </h3>
+                <blockquote>
+                  {{ rejection.details.comments }}
+                </blockquote>
+              </section>
+              <p>
+                {% trans %}
+                  Once you have addressed these concerns, you may resubmit your
+                  app below.
+                {% endtrans %}
+              </p>
+            {% endif %}
+            {{ form_field(form.release_notes, opt=True) }}
+            <p><button type="submit">{{ _('Resubmit App') }}</button></p>
+          </form>
        {% endif %}
      </p>
      <p class="version-status-actions listing-footer">
--- a/mkt/developers/tests/test_views_versions.py
+++ b/mkt/developers/tests/test_views_versions.py
@ -1,4 +1,3 @@
-from nose.plugins.skip import SkipTest
 from nose.tools import eq_
 from pyquery import PyQuery as pq
 import waffle
@ -6,6 +5,7 @@ import waffle
 import amo
 import amo.tests
 from addons.models import Addon
+from users.models import UserProfile


 class TestAppStatus(amo.tests.TestCase):
@ -13,9 +13,12 @@ class TestAppStatus(amo.tests.TestCase):

    def setUp(self):
        self.client.login(username='admin@mozilla.com', password='password')
-        self.webapp = Addon.objects.get(id=337141)
+        self.webapp = self.get_webapp()
        self.url = self.webapp.get_dev_url('versions')

+    def get_webapp(self):
+        return Addon.objects.get(id=337141)
+
    def test_nav_link(self):
        r = self.client.get(self.url)
        eq_(pq(r.content)('#edit-addon-nav li.selected a').attr('href'),
@ -54,10 +57,38 @@ class TestAppStatus(amo.tests.TestCase):
        self.webapp.update(status=amo.STATUS_PENDING)
        r = self.client.get(self.url)
        eq_(r.status_code, 200)
-        eq_(pq(r.content)('#version-status .status-none').length, 1)
+        doc = pq(r.content)
+        eq_(doc('#version-status .status-none').length, 1)
+        eq_(doc('#rejection').length, 0)

    def test_public(self):
        eq_(self.webapp.status, amo.STATUS_PUBLIC)
        r = self.client.get(self.url)
        eq_(r.status_code, 200)
-        eq_(pq(r.content)('#version-status .status-fully-approved').length, 1)
+        doc = pq(r.content)
+        eq_(doc('#version-status .status-fully-approved').length, 1)
+        eq_(doc('#rejection').length, 0)
+
+    def test_rejected(self):
+        comments = "oh no you di'nt!!"
+        amo.set_user(UserProfile.objects.get(username='editor'))
+        amo.log(amo.LOG.REJECT_VERSION, self.webapp,
+                self.webapp.current_version, user_id=999,
+                details={'comments': comments, 'reviewtype': 'pending'})
+        self.webapp.update(status=amo.STATUS_REJECTED)
+
+        r = self.client.get(self.url)
+        eq_(r.status_code, 200)
+        doc = pq(r.content)('#version-status')
+        eq_(doc('.status-rejected').length, 1)
+        eq_(doc('#rejection').length, 1)
+        eq_(doc('#rejection blockquote').text(), comments)
+
+        my_reply = 'fixed just for u, brah'
+        r = self.client.post(self.url, {'release_notes': my_reply})
+        self.assertRedirects(r, self.url, 302)
+
+        webapp = self.get_webapp()
+        eq_(webapp.status, amo.STATUS_PENDING,
+            'Reapplied apps should get marked as pending')
+        eq_(unicode(webapp.versions.all()[0].releasenotes), my_reply)
--- a/mkt/developers/views.py
+++ b/mkt/developers/views.py
@ -33,6 +33,7 @@ from addons import forms as addon_forms
 from addons.decorators import can_become_premium
 from addons.models import Addon, AddonUser
 from addons.views import BaseFilter
+from devhub.models import AddonLog
 from lib.video import library as video_library
 from mkt.developers.decorators import dev_required
 from mkt.developers.forms import (AppFormBasic, AppFormDetails, AppFormMedia,
@ -183,8 +184,27 @@ def disable(request, addon_id, addon):

@dev_required(webapp=True)
 def status(request, addon_id, addon, webapp=False):
-    return jingo.render(request, 'developers/apps/status.html',
-                        {'addon': addon, 'webapp': webapp})
+    form = forms.AppAppealForm(request.POST, product=addon)
+
+    if request.method == 'POST' and form.is_valid():
+        form.save()
+        messages.success(request, _('App successfully resubmitted.'))
+        return redirect(addon.get_dev_url('versions'))
+
+    ctx = {'addon': addon, 'webapp': webapp, 'form': form}
+
+    if addon.status == amo.STATUS_REJECTED:
+        try:
+            entry = (AddonLog.objects
+                     .filter(addon=addon,
+                             activity_log__action=amo.LOG.REJECT_VERSION.id)
+                     .order_by('-created'))[0]
+        except IndexError:
+            entry = None
+        # This contains the rejection reason and timestamp.
+        ctx['rejection'] = entry and entry.activity_log
+
+    return jingo.render(request, 'developers/apps/status.html', ctx)


@dev_required(owner_for_post=True, webapp=True)
--- a/mkt/reviewers/templates/reviewers/review.html
+++ b/mkt/reviewers/templates/reviewers/review.html
@ -56,25 +56,24 @@
  {% include 'reviewers/includes/details.html' %}
 </section>

-{# TODO: List all versions of manifest (bug 741970). #}
-<div id="review-files-header">
-  <h3 id="history">
-    {{ _('App History') }}
-    <span>
-      {% trans %}
-      open:
-      <a class="eh_open" data-num="1" href="#">1</a> &middot;
-      <a class="eh_open" data-num="3" href="#">3</a> &middot;
-      <a class="eh_open" data-num="20" href="#">all</a>
-      {% endtrans %}
-    </span>
-  </h3>
-  <div id="review-files-paginate">
-    {% include 'editors/includes/paginator_history.html'  %}
-  </div>
-</div>
-<div class="results">
-  <div class="results-inner">
+  <div id="history" class="island c">
+    {# TODO: List all versions of manifest (bug 741970). #}
+    <div id="review-files-header">
+      <h3>
+        {{ _('App History') }}
+        <span>
+          {% trans %}
+          open:
+          <a class="eh_open" data-num="1" href="#">1</a> &middot;
+          <a class="eh_open" data-num="3" href="#">3</a> &middot;
+          <a class="eh_open" data-num="20" href="#">all</a>
+          {% endtrans %}
+        </span>
+      </h3>
+      <div id="review-files-paginate">
+        {% include 'editors/includes/paginator_history.html'  %}
+      </div>
+    </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] %}
@ -86,40 +85,6 @@
        </th>
      </tr>
      <tr class="listing-body">
-        <td class="files">
-          <div><strong>{{ _('Files in this version:') }}</strong></div>
-          <ul>
-          {# Hide files until files and versions make sense for apps. #}
-          {% set version_files = [] if product.is_webapp() else version.all_files %}
-          {% for file in version_files %}
-          <li class="file-info">
-            <span class="light">
-              <strong><a href="{{ file.get_url_path('editor') }}" class="install"
-                  data-type="{{ amo.ADDON_SLUGS[product.type] }}">{{ file.platform }}</a></strong>
-              <div>
-                {{ file_review_status(addon, file) }}
-              </div>
-              <a href="{{ url('devhub.file_validation', product.slug, file.id) }}">{{ _('Validation') }}</a>
-              &middot;
-              <a href="{{ url('files.list', file.id) }}">{{ _('Contents') }}</a>
-              {% if show_diff %}
-              &middot;
-              <a class="compare" href="{{ url('files.compare', file.id, file_compare(file, show_diff)) }}">{{ _('Compare') }}</a>
-              {% endif %}
-            </span>
-          </li>
-          {% endfor %}
-          </ul>
-          <div><strong>{{ _('Compatibility:') }}</strong></div>
-          <ul>
-            {% for app, compat in version.compatible_apps_ordered %}
-            <li>
-              <div class="app-icon ed-sprite-{{ app.short }}" title="{{ app.pretty }}"></div>
-              {{ compat }}
-            </li>
-            {% endfor %}
-          </ul>
-        </td>
        <td>
          <table class="activity">
            {% if version.releasenotes %}
@ -145,7 +110,20 @@
            {% set records = version.all_activity %}
            {% for record_version in records %}
              {% set record = record_version.activity_log %}
-              {% include 'editors/includes/history.html' %}
+              <tr>
+                <th>
+                  {{ record.log.short }}
+                </th>
+                <td>
+                  {% trans user=record.user|user_link,
+                           date=record.created|babel_datetime %}
+                    <div>By {{ user }} on {{ date }}</div>
+                  {% endtrans %}
+                  {% if record.details %}
+                    <blockquote>{{ record.details.comments }}</blockquote>
+                  {% endif %}
+                </td>
+              </tr>
            {% endfor %}
            {% if not version.releasenotes and not version.approvalnotes and not records %}
            <tr>
@ -160,7 +138,6 @@
      {% endfor %}
    </table>
  </div>
-</div>

 <form method="POST" action="#review-actions">
  {{ csrf() }}
--- a/mkt/reviewers/tests/test_views.py
+++ b/mkt/reviewers/tests/test_views.py
@ -134,7 +134,7 @@ class TestReviewApp(AppReviewerTest, EditorTest):

    def test_reject(self):
        self.post({'action': 'reject', 'comments': 'suxor'})
-        eq_(self.get_app().status, amo.STATUS_NULL)
+        eq_(self.get_app().status, amo.STATUS_REJECTED)
        action = amo.LOG.REJECT_VERSION
        eq_(ActivityLog.objects.filter(action=action.id).count(), 1)

--- a/mkt/reviewers/utils.py
+++ b/mkt/reviewers/utils.py
@ -182,7 +182,7 @@ class ReviewApp(ReviewBase):

    def process_sandbox(self):
        """Reject an app."""
-        self.set_addon(status=amo.STATUS_NULL)
+        self.set_addon(status=amo.STATUS_REJECTED)
        self.set_files(amo.STATUS_DISABLED, self.version.files.all(),
                       hide_disabled_file=True)