Merge lp:~wallyworld/launchpad/vouchers-timeout2-1014641 into lp:launchpad

Proposed by Ian Booth
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 16181
Proposed branch: lp:~wallyworld/launchpad/vouchers-timeout2-1014641
Merge into: lp:launchpad
To merge this branch: bzr merge lp:~wallyworld/launchpad/vouchers-timeout2-1014641
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+130719@code.launchpad.net

This proposal supersedes a proposal from 2012-10-16.

Commit message

Move the voucher redemption code to a garbo job, making the process to redeem vouchers more robust to salesforce issues.

Description of the change

== Implementation ==

This is a resubmission since the first attempt failed QA.

This branch changes the workflow of how vouchers are redeemed. The redemption via Salesforce has been moved from in-line to a garbo job. Viewing highlights:

- redeeming a voucher updates/creates the commercial subscription record and sets the sales_force_id to 'pending-xxxxx' where xxxx is the voucher id.
- the voucher is not redeemed immediately in salesforce
- the +vouchers view has been changed to remove from the salesforce unredeemed vouchers list any pending vouchers so that someone can not accidentally try and re-redeem a pending voucher
- any attempt to redeem a voucher which has already been submitted (because someone else got in first) results in an field error being displayed on the form
- a garbo job runs frequently to redeem pending vouchers in salesforce and update the commercial subscription record in Launchpad

This version of the branch adds extra functionality to allow the salesforce proxy to specify a timeout when making its RPCs. Previously, the salesforce proxy was only used within Launchpad, and so the lp default request timeout was used. Now, the proxy can be used within a garbo job, where no default timeout is set. I've introduced a new [commercial] configuration setting: voucher_proxy_timeout. This compliments the existing settings voucher_proxy_url, voucher_proxy_port etc. The value is set to 60s. I like the idea of a separately configured timeout for salesforce, rather than reusing the lp timeout, since they are separate systems.

So now the salesforce proxy initialises it's transport with the configured timeout. A little extra infrastructure was required to allow the @with_timeout decorator to get the timeout value from an instance variable on the calling class rather than a hard coded value.

== Tests ==

Add test for new garbo job.
Add tests for new view behaviour regarding not showing pending vouchers and handling submission of vouchers which have been already processed
Update some doctests which repeatedly submit the same voucher id for different projects because this is no longer allowed.
Update timeout.txt to test the new timeout functionality.

== Lint ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  configs/development/launchpad-lazr.conf
  database/schema/security.cfg
  lib/lp/registry/errors.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_commercialsubscription.py
  lib/lp/registry/doc/commercialsubscription.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/model/product.py
  lib/lp/registry/stories/vouchers/xx-voucher-redemption.txt
  lib/lp/registry/templates/person-vouchers.pt
  lib/lp/scripts/garbo.py
  lib/lp/scripts/tests/test_garbo.py
  lib/lp/services/timeout.py
  lib/lp/services/config/schema-lazr.conf
  lib/lp/services/salesforce/proxy.py
  lib/lp/services/webapp/doc/timeout.txt
  lib/lp/testing/factory.py

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote : Posted in a previous version of this proposal

Remove the submit-twice-error you added to lib/lp/registry/doc/commercialsubscription.txt because you wrote a unit test for it.

I see the template is doing python evaluation twice to chose true ans false outcomes
    condition="python: vouchers or view_errors"
This check (or both) can be moved into the view so that python: is not used in TAL where is it doing late compilation.

review: Approve (code)
Revision history for this message
Ian Booth (wallyworld) wrote : Posted in a previous version of this proposal

> Remove the submit-twice-error you added to
> lib/lp/registry/doc/commercialsubscription.txt because you wrote a unit test
> for it.
>

The tests are subtly different. The doc test is a unit test for the model object and the raising of the VoucherAlreadyRedeemed exception. The unit test is for the view layer and checks that the model exception is handled correctly.

Revision history for this message
Curtis Hovey (sinzui) wrote : Posted in a previous version of this proposal

Thank you for clarifying the tests.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you for this fix. Your changes look obvious now, but I could not see how to solve the problem last Friday.

review: Approve (code)