Merge lp:~ricardokirkner/locolander/merge-request-views into lp:locolander

[Ricardo Kirkner]

added view for triggering a merge request run

[Natalia Bidart (nataliabidart)]

Branch looks great. There is only one blocker, and is in the merge_request_run view: the get_object_or_404 should ensure that the user currently logged is the owner of the project that holds the given merge request (otherwise anyone logged in could visit this url by knowing the MR id).

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'locolander/locolanderweb/templates/locolanderweb/project/detail.html'
--- locolander/locolanderweb/templates/locolanderweb/project/detail.html	2013-07-06 23:19:29 +0000
+++ locolander/locolanderweb/templates/locolanderweb/project/detail.html	2013-07-20 21:25:25 +0000
@@ -4,6 +4,12 @@
 <div class="project-detail">
 {% if project %}
     <h1>Project {{ project.name }}</h1>
+    <form action="." method="POST">
+        {% csrf_token %}
+        <div class="actions">
+            <input type="submit" value="Scan this project" />
+        </div>
+    </form>
 
     {% if project.mergerequest_set.count %}
     {% for mr in project.mergerequest_set.all %}
@@ -12,14 +18,29 @@
         <ul>
             <li>Source: {{ mr.source }}</li>
             <li>Target: {{ mr.target }}</li>
+            <li>Author: {{ mr.author }}</li>
             <li>Reviewers: {{ mr.reviewers }}</li>
             <li>Created: {{ mr.date_created }}</li>
             <li>Status: {{ mr.status }}</li>
         </ul>
+        <form action="{% url 'request-run' mr.id %}" method="POST">
+            {% csrf_token %}
+            <div class="actions">
+                <input type="submit" value="Land now!" />
+            </div>
+        </form>
+
         {% if mr.runlog_set.count %}
-        <pre style="background: #eeeeee;">
-        {{ mr.runlog_set.latest.timestamp }}
-        </pre>
+        {% for entry in mr.runlog_set.all reversed %}
+        <div class="runlog-entry">
+            <span>
+            {{ entry.timestamp }}
+            </span>
+            <pre style="background: #eeeeee;">
+            {{ entry.raw|wordwrap:120 }}
+            </pre>
+        </div>
+        {% endfor %}
         {% endif %}
     </div>
     {% endfor %}
@@ -28,12 +49,6 @@
         processed merge requests.</h2>
     {% endif %}
 
-    <form action="." method="POST">
-        {% csrf_token %}
-        <div class="actions">
-            <input type="submit" value="Scan this project" />
-        </div>
-    </form>
 </div>
 {% endif %}
 {% endblock content %}

=== modified file 'locolander/locolanderweb/tests/test_views.py'
--- locolander/locolanderweb/tests/test_views.py	2013-07-06 22:26:19 +0000
+++ locolander/locolanderweb/tests/test_views.py	2013-07-20 21:25:25 +0000
@@ -1,4 +1,6 @@
 from django.core.urlresolvers import reverse
+from django.utils import html
+from mock import ANY, patch
 from pyquery import PyQuery
 
 from locolanderweb.forms import ProjectForm
@@ -77,3 +79,78 @@
         response = self.client.get(self.url)
 
         self.assertContains(response, 'Project %s' % self.project.name)
+
+    def test_detail_does_not_include_runlog(self):
+        response = self.client.get(self.url)
+
+        self.assertNotContains(response, '<div class="runlog-entry">')
+
+    def test_detail_includes_runlog(self):
+        mr = self.factory.make_merge_request(project=self.project)
+        self.factory.make_run_log(merge_request=mr)
+        self.factory.make_run_log(merge_request=mr)
+        response = self.client.get(self.url)
+
+        self.assertContains(response, '<div class="runlog-entry">', count=2)
+
+    def test_detail_includes_land_now_button(self):
+        merge_request = self.factory.make_merge_request(project=self.project)
+        response = self.client.get(self.url)
+
+        url = reverse('request-run', args=[merge_request.id])
+        self.assertContains(response, '<form action="%s" method="POST">' % url)
+
+    def test_details_includes_author(self):
+        mr = self.factory.make_merge_request(project=self.project)
+
+        response = self.client.get(self.url)
+
+        author_html = "Author: %s" % html.escape(mr.author)
+        self.assertContains(response, author_html)
+
+
+class MergeRequestRunTestCase(BaseTestCase):
+
+    def setUp(self):
+        super(MergeRequestRunTestCase, self).setUp()
+        self.user = self.factory.make_user(password=self.password)
+        assert self.client.login(username=self.user.username,
+                                 password=self.password)
+        self.project = self.factory.make_project(user=self.user)
+        self.merge_request = self.factory.make_merge_request(
+            project=self.project)
+        self.url = reverse('request-run',
+                           kwargs=dict(merge_request_id=self.merge_request.id))
+
+    def test_get(self):
+        response = self.client.get(self.url)
+        self.assertTemplateUsed(response, 'locolanderweb/project/detail.html')
+        self.assertEqual(response.context['project'],
+                         self.project)
+
+    def test_get_not_existing(self):
+        url = reverse('request-run', kwargs=dict(merge_request_id=99))
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 404)
+
+    def test_get_non_owner(self):
+        # make new user
+        user = self.factory.make_user(password=self.password)
+        # login using non-owner user
+        self.client.login(username=user.username, password=self.password)
+
+        response = self.client.get(self.url)
+        self.assertEqual(response.status_code, 404)
+
+    @patch('locolanderweb.views.messages.info')
+    @patch('locolanderweb.tasks.run_request.delay')
+    def test_post(self, mock_delay, mock_info):
+        response = self.client.post(self.url)
+        mock_delay.assert_called_once_with(self.merge_request)
+        msg = 'Merge of {mr.source} into {mr.target} requested.'
+        mock_info.assert_called_once_with(ANY,
+                                          msg.format(mr=self.merge_request))
+        self.assertRedirects(
+            response,
+            reverse('project-detail',
+                    kwargs=dict(project_id=self.project.id)))

=== modified file 'locolander/locolanderweb/urls.py'
--- locolander/locolanderweb/urls.py	2013-07-06 23:19:29 +0000
+++ locolander/locolanderweb/urls.py	2013-07-20 21:25:25 +0000
@@ -13,4 +13,6 @@
     url(r'^projects/add/$', 'project_add', name='project-add'),
     url(r'^projects/(?P<project_id>\d+)/$', 'project_detail',
         name='project-detail'),
+    url(r'requests/(?P<merge_request_id>\d+)/run/$', 'merge_request_run',
+        name='request-run'),
 )

=== modified file 'locolander/locolanderweb/views.py'
--- locolander/locolanderweb/views.py	2013-07-06 23:19:29 +0000
+++ locolander/locolanderweb/views.py	2013-07-20 21:25:25 +0000
@@ -5,8 +5,9 @@
 from django.shortcuts import get_object_or_404
 from django.template.response import TemplateResponse
 
-from locolanderweb.models import Project
+from locolanderweb import tasks
 from locolanderweb.forms import ProjectForm
+from locolanderweb.models import MergeRequest, Project
 
 
 @login_required
@@ -48,11 +49,28 @@
 
     context = dict(project=project)
     if request.method == 'POST':
-        # scan, this is a fake now
-        project.approved_requests()
+        tasks.scan_project.delay(project)
         messages.info(request, 'Scan for %s requested.' % project.name)
         return HttpResponseRedirect(reverse('project-detail',
                                     kwargs=dict(project_id=project.id)))
 
     return TemplateResponse(
         request, 'locolanderweb/project/detail.html', context=context)
+
+
+@login_required
+def merge_request_run(request, merge_request_id):
+    merge_request = get_object_or_404(
+        MergeRequest, id=merge_request_id, project__owner=request.user)
+    project = merge_request.project
+
+    context = dict(project=project)
+    if request.method == 'POST':
+        tasks.run_request.delay(merge_request)
+        msg = 'Merge of {mr.source} into {mr.target} requested.'
+        messages.info(request, msg.format(mr=merge_request))
+        return HttpResponseRedirect(
+            reverse('project-detail', kwargs=dict(project_id=project.id)))
+
+    return TemplateResponse(
+        request, 'locolanderweb/project/detail.html', context=context)