launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

The proposal to merge lp:~wallyworld/launchpad/delete-bugtask-ui-ajax-878909 into lp:launchpad has been updated.

Description changed to:

== Implementation ==

Requires feature flag: disclosure.delete_bugtask.enabled

1. The core ajax implementation is relatively straightforward:
- capture the click on the delete link
- perform a POST with url = <bug task url>/+delete
- the standard zope BugTaskDeleteView form performs the delete as for a HTML form submit
- the form detects an XHR request being used and renders and returns the HTML for the new bug tasks table
- the javascript caller replaces the old bugtasks table with the new HTML received from the XHR call

Some small refactoring was required to extract the tales required to render just the bug tasks table. The existing view called +bugtasks-and-nominations-table was renamed to +bugtasks-and-nominations-portal since it renders Affects Me Too and Also Affacts links as well as the table. The +bugtasks-and-nominations-table view renders just the bugtasks table. The tales used for the portal references the newly created table view, allowing the tales to be reused.

So first issue: the new bugtasks table is duly rendered but none of the javascript widgets are wired up. This is because some of the javascript is included as part of the tales used to render the picker widgets, while other javascript is only executed once on page load.

2. Extract bugtask row javascript from tales

The lp.bug.bugtak_index module already contains a method (setup_bugtask_row) for setting up a bug task row (wiring up importance/status widgets, adding expander etc). So the javascript from the bugtask-tasks-nominations-and-table-row.pt tales was moved into this existing method. The BugTaskTableRowView view had a js_config method which provided data for the javascript embedded in the tales. This was renamed to bugtask_config and was re-purposed to stuff data into the client cache, allowing the moved javascript to get at the data it needs.

An onload handler was added to execute a new setup_bugtask_table() method. This uses the client cache data and the newly refactored javascript to wire up the bug tasks table.

When the new bug tasks table is rendered after the XHR delete call, the same setup_bugtask_table() is called to do the wiring.

3. Bug found

There were conflicting implementations of userCanEditImportance() found. One was essentially:

bugtask.userCanEditImportance(self.user) and not bugtask.bugwatch

while the other left out the second bugwatch condition. This caused the edit icon to be incorrectly rendered for remote bug tasks. I've fixed it.

4. Wire the pickers

There's pickers for assignee and also project/source package selection in the expandable form for each bug task. The javascript to wire these up lives in the form-picker-macro.pt tales. So it's not feasible to rip this out and reuse as per item #2 above since this picker stuff is generic infrastructure and things would break. So I chose the following solution. The javascript function to do the picker wiring is registered in a new yui namespace "lp.app.picker.connect". The function is still run as per the current picker infrastructure but the function is also available to run later as and when required. So when the html for the new table is rendered, the picker widgets embedded in the table are re-wired using the previously registered functions.

5. Confirmation dialog

A confirmation dialog was added to guard against accidental deletion.

6. Problem - deleting the highlighted bug task

The page used to display list of bug tasks for a given bug is rendered as the bug index view as well as the index view for each individual bug task url. The bug task corresponding to the current url is highlighted and:
i) the client cache self_link and web_link values are for the highlighted bugtask
ii) the XHR links for duplicate marking, privacy setting etc are all relative to this url

When the current bug task is deleted, the table is correctly re-rendered and the bug's new default bug tasks is now the highlighted one. But the urls referred to above in the cache and links are now invalid.

One existing case was fixed - the subscribers portlet setup was changed to use the bug's url rather than the bug task.
The other cases need fixing though.

Options:
i) update the URLs using javascript code when the bug task table is re-rendered
ii) server side zope publisher redirect when invoked with the old bug task urls
iii) when highlighted bug task is deleted, in that case simply redirect to the bugtask url of the new default task. this causes a new page load but the urls will be correct

Option (iii) is the one that can be best implemented. It has its own problem though. The the server does a redirect during an XHR call, the redirect is followed and the rendered contents are returned to the Javascript caller with status 200. The caller interprets this data as if the original call completed without the redirect. So the new bugtask page is rendered over the top of the bugtask table. Not what we want.

To solve the problem, the view returns a JSON dict containing the new bugtask URL when a redirection is required. The caller then does the redirect itself. The existing ReturnToReferrerMixin class was used to determine the referring URL and hence whether the bugtask being deleted is the current one and hence whether redirection is required. There is a slight pause when the redirect happens but I'm not sure I can do much about that.

== Demo and QA ==

http://people.canonical.com/~ianb/delete_bugtask.ogv

== Tests ==

1. Renamed +bugtasks-and-nominations-table view

Add extra test to bug-views.txt and update existing tests to use the new view name.

2. New setup for bugs.javascript.subscribers.js

Update test_subscribers.js

3. New DeleteBugTaskView behaviour

Add new tests to TestDeleteBugTaskView to check the ajax behaviour:
- test_ajax_delete_non_current_bugtask
- test_ajax_delete_current_bugtask

4. New Javascript bugtask delete functionality

Add new yui test: bugs.javascript.tests.test_bugtask_delete.js

5. Under the covers picker changes etc

Reply on yuixhr tests ported from windmill tests (when done) since the windmill tests covered the rendering and wiring up of the pickers.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/templates/form-picker-macros.pt
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/configure.zcml
  lib/lp/bugs/browser/tests/bug-views.txt
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/subscribers.js
  lib/lp/bugs/javascript/tests/test_bugtask_delete.html
  lib/lp/bugs/javascript/tests/test_bugtask_delete.js
  lib/lp/bugs/javascript/tests/test_subscribers.js
  lib/lp/bugs/templates/bugtask-index.pt
  lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt
  lib/lp/bugs/templates/bugtasks-and-nominations-portal.pt
  lib/lp/bugs/templates/bugtasks-and-nominations-table.pt

./lib/lp/bugs/javascript/bugtask_index.js
     640: Move 'var' declarations to the top of the function.
     640: Stopping.  (46% scanned).
      -1: JSLINT had a fatal error.

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-ui-ajax-878909/+merge/80779
-- 
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-ui-ajax-878909/+merge/80779
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-bugtask-ui-ajax-878909 into lp:launchpad.