Merge lp:~psivaa/uci-engine/find-missing-mps into lp:uci-engine

[Para Siva]

Warning the user about removal of MP's and Sources from subtickets.

This is the first part of stray sources and MP removal task, to warn the user about missing sources and MP's whilst editing. The warning comes as a modal panel with missing entries and for now clicking 'OK' on the panel will complete the editing but will not actually remove the entries from the subtickets. This will be done in a subsequent MP.

[Francis Ginther (fginther)]

Most of my comments are cosmetic and refactoring suggestions. The two that I'd like some more feedback on are the test cases. These tests modify the field contents, but I don't see that anything is done to verify that it had any effect or the warning message was accurate. They appear to be useful tests otherwise, just missing an assert on the warning message.

I do see the goal of this MP and the desire to put up a warning. It makes sense and I think is ultimately more user friendly then silently deleting them as the spreadsheet would do.

review: Needs Fixing

[lp:~psivaa/uci-engine/find-missing-mps]

917. By Para Siva on 2014-12-19

Check for the content of warning message

[Francis Ginther (fginther)]

These tests are improved, thanks.

I'm now hitting a test failure in test_edit_ticket_sources_and_mps(), which only performs a ticket edit test and relies on a prior test to create the ticket. These new tests introduce changes to the last ticket created and leads to failure in test_edit_ticket_sources_and_mps(). Also, with all of the MPs that have been targeting this file, it's been constantly churning and the order of tests changing. At a minimum test_edit_ticket_sources_and_mps() needs to be updated, a better solution to remove dependencies between tests would be to refactor these into idempotent tests. (The tests being added here are idempotent).

The pep8 error needs to be fixed in this MP, but test_edit_ticket_sources_and_mps() could be fixed in another MP. I may end up doing some refactoring if I find some spare time as I try to update some other MPs I'm trying to land, so let's coordinate. I'm tentatively approving so as not to hold up tarmac if we refactor that test in another MP and the pep8 error gets resolved.

review: Approve

[Francis Ginther (fginther)]

Argh! I just realized that the required fields change landed this week and the two tests need to be updated with the latest trunk. 'owner' is no longer a required field, 'landers', 'test_notes' and one of ['sources', 'merge_proposals'] are now required.

Again, we may be able to coordinate on refactoring these tests.

review: Needs Fixing

[lp:~psivaa/uci-engine/find-missing-mps]

918. By Para Siva on 2015-01-03

Refactor code, make old tests idempotent before merging trunk

919. By Para Siva on 2015-01-04

Merging trunk and resolving conflict before fixing the tests

920. By Para Siva on 2015-01-04

Fixing tests after merging trunk

[Para Siva (psivaa)]

Thanks for the review. Rev 920 has the fixes to the comments on rev916 and it also fixes to the tests after merging with trunk where certain fields made 'required'.

[Para Siva (psivaa)]

917 was an intermediate push :), but thanks again for the reviews. r920 is the latest.

Unmerged revisions

920. By Para Siva on 2015-01-04

Fixing tests after merging trunk

919. By Para Siva on 2015-01-04

Merging trunk and resolving conflict before fixing the tests

918. By Para Siva on 2015-01-03

Refactor code, make old tests idempotent before merging trunk

917. By Para Siva on 2014-12-19

Check for the content of warning message

916. By Para Siva on 2014-12-17

Add tests make them pas

915. By Para Siva on 2014-12-17

some fancy refactoring

914. By Para Siva on 2014-12-16

Making panel look saner

913. By Para Siva on 2014-12-16

Add a header to the panel

912. By Para Siva on 2014-12-16

Include source packages for the missing

911. By Para Siva on 2014-12-16

Remove assignable conflict

=== modified file 'tests/test_webui.py' (properties changed: +x to -x)
--- tests/test_webui.py	2014-12-18 21:37:15 +0000
+++ tests/test_webui.py	2015-01-04 15:25:56 +0000
@@ -249,6 +249,24 @@
             [s.text.splitlines() for s in subtickets])
 
     def test_edit_ticket(self):
+        go_to('/create')
+        wait_for(get_element, tag='form')
+
+        # Submit the form with the required fields.
+        title = get_element_by_css('input[name="title"]')
+        title_text = 'Editing with Invalid MPs at {}'.format(time.time())
+        write_textfield(title, title_text)
+        landers = get_element_by_css('input[name="landers"]')
+        write_textfield(landers, '[e-mail]')
+        test_notes = get_element_by_css('textarea[name="test_notes"]')
+        write_textfield(test_notes, 'test this')
+        sources = get_element_by_css('textarea[name="sources"]')
+        write_textfield(sources, 'foo, , ,  bar\nbang , ')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        # Redirects to the ticket page.
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(title_text, ticket_title.text)
         # Edit a ticket via WebUI form.
         go_to('/')
 
@@ -313,6 +331,24 @@
         fails(get_element_by_css, 'a.bug-link')
 
     def test_edit_ticket_sources_and_mps(self):
+        go_to('/create')
+        wait_for(get_element, tag='form')
+
+        # Submit the form with the required fields.
+        title = get_element_by_css('input[name="title"]')
+        title_text = 'Editing with Invalid MPs at {}'.format(time.time())
+        write_textfield(title, title_text)
+        landers = get_element_by_css('input[name="landers"]')
+        write_textfield(landers, '[e-mail]')
+        test_notes = get_element_by_css('textarea[name="test_notes"]')
+        write_textfield(test_notes, 'test this')
+        sources = get_element_by_css('textarea[name="sources"]')
+        write_textfield(sources, 'foo, , ,  bar\nbang , ')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        # Redirects to the ticket page.
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(title_text, ticket_title.text)
         # Edit a ticket sources & MPs.
         go_to('/')
 
@@ -330,7 +366,7 @@
         title = get_element_by_css('input[name="title"]')
         write_textfield(title, new_title)
         sources = get_element_by_css('textarea[name="sources"]')
-        write_textfield(sources, 'boing')
+        write_textfield(sources, 'boing, foo, bar, bang')
         mps = get_element_by_css('textarea[name="merge_proposals"]')
         write_textfield(
             mps,
@@ -345,49 +381,41 @@
         self.assertEqual(new_title, ticket_title.text)
         subtickets = get_elements_by_css('table.subticket')
         self.assertEqual(
-            [['Source: foo',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs: https://code.launchpad.net/~x/foo/a_branch/+merge/1',
-              'https://code.launchpad.net/~x/foo/a_branch/+merge/2',
-              'Artifacts:',
-              'Conflicts:'],
-             ['Source: bar',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs:',
-              'Artifacts:',
-              'Conflicts:'],
-             ['Source: bang',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs:',
-              'Artifacts:',
-              'Conflicts:'],
-             ['Source: baz',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs: https://code.launchpad.net/~x/baz/a_branch/+merge/3',
-              'Artifacts:',
-              'Conflicts:'],
-             ['Source: boing',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs:',
-              'Artifacts:',
-              'Conflicts:'],
-             ['Source: zoing',
-              'Status: -',
-              'Step: New',
-              'Current version: Not available',
-              'MPs: https://code.launchpad.net/~x/zoing/a_branch/+merge/1',
-              'Artifacts:',
-              'Conflicts:']],
+            [[u'Source: foo',
+              u'Status: -',
+              u'Step: New',
+              u'Current version: Not available',
+              u'MPs:',
+              u'Artifacts:',
+              u'Conflicts:'],
+             [u'Source: bar',
+              u'Status: -',
+              u'Step: New',
+              u'Current version: Not available',
+              u'MPs:',
+              u'Artifacts:',
+              u'Conflicts:'],
+             [u'Source: bang',
+              u'Status: -',
+              u'Step: New',
+              u'Current version: Not available',
+              u'MPs:',
+              u'Artifacts:',
+              u'Conflicts:'],
+             [u'Source: boing',
+              u'Status: -',
+              u'Step: New',
+              u'Current version: Not available',
+              u'MPs:',
+              u'Artifacts:',
+              u'Conflicts:'],
+             [u'Source: zoing',
+              u'Status: -',
+              u'Step: New',
+              u'Current version: Not available',
+              u'MPs: https://code.launchpad.net/~x/zoing/a_branch/+merge/1',
+              u'Artifacts:',
+              u'Conflicts:']],
             [s.text.splitlines() for s in subtickets])
 
     def test_create_first_with_invalid_mps(self):
@@ -397,7 +425,7 @@
 
         # Submit the form with the required fields.
         title = get_element_by_css('input[name="title"]')
-        title_text = 'Testing Invalid MPs at {}'.format(time.asctime())
+        title_text = 'Testing Invalid MPs at {}'.format(time.time())
         write_textfield(title, title_text)
         landers = get_element_by_css('input[name="landers"]')
         write_textfield(landers, '[e-mail]')
@@ -434,7 +462,7 @@
 
         # Submit the form with the required fields.
         title = get_element_by_css('input[name="title"]')
-        title_text = 'Editing with Invalid MPs at {}'.format(time.asctime())
+        title_text = 'Editing with Invalid MPs at {}'.format(time.time())
         write_textfield(title, title_text)
         landers = get_element_by_css('input[name="landers"]')
         write_textfield(landers, '[e-mail]')
@@ -462,7 +490,7 @@
 
         # Update the ticket with a unique title and distinct sources
         # and merge proposals.
-        new_title = 'Edited for testing on {}!'.format(time.asctime())
+        new_title = 'Edited for testing on {}!'.format(time.time())
         title = get_element_by_css('input[name="title"]')
         write_textfield(title, new_title)
         sources = get_element_by_css('textarea[name="sources"]')
@@ -478,6 +506,121 @@
             'The entry https://bazaar.launchpad.net/~x/zoing/a_branch/+merge/5'
             ' is not a valid MP URL.', error.text)
 
+    def test_edit_by_removing_mps(self):
+        # Create a ticket using sources and MPs.
+        go_to('/create')
+        wait_for(get_element, tag='form')
+
+        # Submit the form with the required fields.
+        title_text = 'Editing with Invalid MPs at {}'.format(time.time())
+        title = get_element_by_css('input[name="title"]')
+        title_text = 'Remove MP test at {}'.format(time.time())
+        write_textfield(title, title_text)
+        landers = get_element_by_css('input[name="landers"]')
+        write_textfield(landers, '[e-mail]')
+        sources = get_element_by_css('textarea[name="sources"]')
+        write_textfield(sources, 'testsource')
+        test_notes = get_element_by_css('textarea[name="test_notes"]')
+        write_textfield(test_notes, 'test this')
+        mps = get_element_by_css('textarea[name="merge_proposals"]')
+        write_textfield(
+            mps,
+            'https://code.launchpad.net/~x/foo/a_branch/+merge/1\n'
+            'https://code.launchpad.net/~x/foo/a_branch/+merge/2\n'
+            'https://code.launchpad.net/~x/baz/a_branch/+merge/3')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        # Redirects to the ticket page.
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(title_text, ticket_title.text)
+
+        # Edit a ticket sources & MPs.
+        go_to('/')
+
+        # Edit the first available ticket.
+        wait_for(get_element_by_css, 'a.yui3-pagview-link-page-active')
+        a_ticket_link = get_elements_by_css('a.row_link')[0]
+        a_ticket_link.click()
+        edit_link = wait_for(get_element_by_css, 'a.edit-link')
+        edit_link.click()
+        wait_for(get_element, tag='form')
+
+        # Update the ticket with a unique title and distinct sources
+        # and merge proposals.
+        new_title = 'Edited for testing on {}!'.format(time.time())
+        title = get_element_by_css('input[name="title"]')
+        write_textfield(title, new_title)
+        mps = get_element_by_css('textarea[name="merge_proposals"]')
+        write_textfield(
+            mps,
+            'https://code.launchpad.net/~x/foo/a_branch/+merge/1\n'
+            'https://code.launchpad.net/~x/baz/a_branch/+merge/3\n'
+            'https://code.launchpad.net/~x/baz/a_branch/+merge/4')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        warn_bd = wait_for(get_element_by_css, '.yui3-widget-bd')
+        self.assertIn('https://code.launchpad.net/~x/foo/a_branch/+merge/2',
+                      warn_bd.text)
+        ok_link = get_elements_by_css('.yui3-button')[1]
+        ok_link.click()
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(new_title, ticket_title.text)
+
+    def test_edit_by_removing_sources(self):
+        # Create a ticket using sources and MPs.
+        go_to('/create')
+        wait_for(get_element, tag='form')
+
+        # Submit the form with the required fields.
+        title = get_element_by_css('input[name="title"]')
+        title_text = 'Remove MP test at {}'.format(time.time())
+        write_textfield(title, title_text)
+        owner = get_element_by_css('input[name="landers"]')
+        write_textfield(owner, '[e-mail]')
+        test_notes = get_element_by_css('textarea[name="test_notes"]')
+        write_textfield(test_notes, 'test this')
+        sources = get_element_by_css('textarea[name="sources"]')
+        write_textfield(sources,
+                        'boing\n'
+                        'doing\n'
+                        'going\n'
+                        'noing')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        # Redirects to the ticket page.
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(title_text, ticket_title.text)
+
+        # Edit a ticket sources & MPs.
+        go_to('/')
+
+        # Edit the first available ticket.
+        wait_for(get_element_by_css, 'a.yui3-pagview-link-page-active')
+        a_ticket_link = get_elements_by_css('a.row_link')[0]
+        a_ticket_link.click()
+        edit_link = wait_for(get_element_by_css, 'a.edit-link')
+        edit_link.click()
+        wait_for(get_element, tag='form')
+
+        # Update the ticket with a unique title and distinct sources
+        # and merge proposals.
+        new_title = 'Edited for testing on {}!'.format(time.time())
+        title = get_element_by_css('input[name="title"]')
+        write_textfield(title, new_title)
+        sources = get_element_by_css('textarea[name="sources"]')
+        write_textfield(sources,
+                        'boing\n'
+                        'doing\n'
+                        'noing')
+        submit = get_element_by_css('input[type="submit"]')
+        click_button(submit, wait=True)
+        warn_bd = wait_for(get_element_by_css, '.yui3-widget-bd')
+        self.assertIn('going', warn_bd.text)
+        ok_link = get_elements_by_css('.yui3-button')[1]
+        ok_link.click()
+        ticket_title = wait_for(get_element, tag='h2')
+        self.assertEqual(new_title, ticket_title.text)
+
     def test_json_status(self):
         # Ensure the webui charm is providing us a valid status_urls.json.
         go_to('/static/engine-health/status_urls.json')

=== modified file 'webui/common/static/common/webui.css'
--- webui/common/static/common/webui.css	2014-11-12 03:18:24 +0000
+++ webui/common/static/common/webui.css	2015-01-04 15:25:56 +0000
@@ -132,6 +132,48 @@
   padding: 0px;
 }
 
+.yui3-panel {
+ outline: 1px solid;
+ background-color: white;
+ color: black;
+ text-align: center;
+ margin: 10px 15px 15px 20px;
+ padding: 5px;
+ vertical-align: baseline;
+}
+
+.yui3-widget-bd{
+ background-color: white;
+ color: black;
+ text-align: center;
+ margin: 10px 15px 15px 20px;
+ padding: 5px;
+ vertical-align: baseline;
+}
+
+.yui3-widget-hd{
+ font-weight: bold;
+ text-align: center;
+ color: white;
+ background-color: #c03f11;
+}
+
+.yui3-widget-ft{
+ text-align: center;
+ margin: 0px;
+ padding: 0px;
+ border: 0px none;
+ vertical-align: baseline;
+}
+
+.yui3-widget-buttons{
+ float: center;
+ position: relative;
+ width:200px;
+ border: 5px none;
+ vertical-align: baseline;
+}
+
 #refresh {
   padding: 5px;
 }
@@ -366,6 +408,7 @@
   text-align: left;
 }
 
+
 /* General links decorators. */
 a.image-link {
   background-image: url('/static/common/cdrom.svg');

=== modified file 'webui/tickets/static/tickets/webuiforms.js'
--- webui/tickets/static/tickets/webuiforms.js	2014-12-18 20:27:04 +0000
+++ webui/tickets/static/tickets/webuiforms.js	2015-01-04 15:25:56 +0000
@@ -32,7 +32,7 @@
             {label : 'Trusty - 14.04', value : 'trusty'},
             {label : 'Precise - 12.04', value : 'precise'}
         ],
-
+ 
         /**
          * Convert form 'sources' and 'merge_proposals' to 'subtickets'.
          *
@@ -221,7 +221,7 @@
          *
          * Positive int32 except ZERO (1 - 2147483648)
          *
-         * @method setupEditForm
+         * @method validateBugId
          */
         validateBugId: function(val, field) {
             var filter = /^[\d]*$/;
@@ -256,6 +256,46 @@
         },
 
         /**
+         * Find missing MP entries.
+         *
+         * @method findMissingMps
+         */
+        findMissingMps: function(ticket_data, form_data) {
+            var new_mps = [];
+            form_data['subtickets'].forEach( function(f) {
+                if (f['merge_proposals']){
+                    f['merge_proposals'].forEach (function (m) {
+                        new_mps.push(m['lp_url']);
+                    });
+                }
+            });
+            return Y.webuiforms.findMissed(ticket_data, new_mps);
+        },
+
+        /**
+         * Find findMissingSources
+         */
+        findMissingSources: function(ticket_data, form_data) {
+            var new_sources = [];
+            form_data['subtickets'].forEach( function(f) {
+                new_sources.push(f['sourcepackage']['name']);
+            });
+            return Y.webuiforms.findMissed(ticket_data, new_sources);
+        },
+
+        /**
+         * Compare two arrays and return missing entries.
+         *
+         * @method findMissed
+         */
+        findMissed: function(ticket_data, form_data) {
+            Array.prototype.diff = function(a) {
+                return this.filter(function(i) {return a.indexOf(i) < 0;});
+            };
+            return ticket_data.diff(form_data);
+        },
+
+        /**
          * Setup a form for editing a ticket.
          *
          * @method setupEditForm
@@ -294,6 +334,48 @@
                 ]
             });
 
+            var dialog = new Y.Panel({
+                contentBox : Y.Node.create('<div id="dialog" />'),
+                headerContent: 'Content removal warning',
+                bodyContent: '<div class="message icon-warn">Removal warning</div>',
+                width      : 610,
+                zIndex     : 6,
+                centered   : true,
+                modal      : false, 
+                render     : true,
+                visible    : false, 
+                buttons    : {
+                     footer: [
+                        {
+                            name  : 'cancel',
+                            label : 'Cancel',
+                            action: 'onCancel'
+                        },
+
+                        {
+                            name     : 'proceed',
+                            label    : 'OK',
+                            action   : 'onOK'
+                        }
+                    ]
+                }
+            });
+
+            dialog.onCancel = function (e) {
+                e.preventDefault();
+                this.hide();
+            }
+
+            dialog.onOK = function (e) {
+                e.preventDefault();
+                this.hide();
+                // XXX psivaa 20141216: accept and do nothing for now
+                Y.config.win.location.href = f.get(
+                    'action').replace('/api/v1', '');
+            }
+
+            var mps = [];
+            var sources = [];
             // Load ticket information from TS and pre-fill the form.
             Y.io('/api/v1/fullticket/' + ticket_id + '/', {
                 sync: true,
@@ -313,14 +395,12 @@
                                     'value', (ticket[name] || '').toString());
                                 break;
                             case 'sources':
-                                var sources = [];
                                 ticket.subticket.forEach( function(s) {
                                     sources.push(s.sourcepackage.name);
                                 });
                                 field.set('value', sources.join('\n'));
                                 break;
                             case 'merge_proposals':
-                                var mps = [];
                                 ticket.subticket.forEach( function(s) {
                                     s.merge_proposals.forEach( function(mp) {
                                         mps.push(mp.lp_url);
@@ -350,6 +430,7 @@
 
             Y.one('input[name="edit-button"]').on('click', function(e) {
                 e.halt();
+                
                 if (!f._runValidation()) {
                     return 1;
                 }
@@ -375,6 +456,9 @@
                     form_data['bug_id'] = null;
                 }
 
+                var missingMps = Y.webuiforms.findMissingMps(mps, form_data);
+                var missingSources = Y.webuiforms.findMissingSources(sources, form_data);
+
                 Y.io(f.get('action'), {
                     // XXX cprov 20141204: we only use and support PUT
                     // because of a phantomjs bug (see TicketResource
@@ -388,8 +472,22 @@
                     },
                     on: {
                         success: function(tx, r) {
-                            Y.config.win.location.href = f.get(
-                                'action').replace('/api/v1', '');
+                            if (missingSources.length || missingMps.length ){
+                                    var theMessage = "<p><b>The following have been removed from the original ticket</b></p>";
+                                    if(missingSources.length)
+                                        theMessage += "<p><b>Source(s):</b></p>" + "<p>" + missingSources.join("<br>") + "</p>";
+                                    
+                                    if(missingMps.length)
+                                        theMessage += "<p><b>MP(s):</b></p> " + "<p>" + missingMps.join("<br>") + "</p>";
+                                    
+                                    theMessage += "<p><b> Click OK to accept</b></p>";
+                                    Y.one('#dialog .message').setHTML(theMessage);
+                                    dialog.show();
+                                }
+                            else {
+                                Y.config.win.location.href = f.get(
+                                    'action').replace('/api/v1', '');
+                            }
                         },
                         failure: function (tx, r) {
                             Y.log('FAILURE: ' + r.responseText);
@@ -403,4 +501,4 @@
         }
     };
 }, '0.0.1', {requires: ['io', 'node', 'json-parse', 'json-stringify',
-                        'gallery-form']});
+                        'gallery-form', 'panel']});