SOAP request in a while loop

Question

I am developing a service where the user need to sign the seller agreement with Mobile BankID (Swedish e-sign service). To make this work I need to make SOAP request to get the progress status of the signing request in Mobile BankID.

Is this code understandable? How can I make it more clean?

def collect_request(transaction_id):
    '''You can make a delay for 9 seconds before you start to call the Collect
    method. After 9s you begin to make CollectRequest with a 3 seconds delay
    between each collect. First answer will probably be
    OUTSTANDING_TRANSACTION, then USER_SIGN and at last COMPLETE. (After six
    OUTSTANDING_TRANSACTION you get NO_CLIENT, just to indicate that the user
    not yet has started her BankID client.

    After 180 seconds, you will at last get faultStatus EXPIRED_TRANSACTION'''
    time.sleep(9)
    seller_agreement = SellerAgreement.objects.get(id=transaction_id)
    url = '<WSDL URL HERE>'
    client = Client(url)

    val = 'OUTSTANDING_TRANSACTION'
    while True:
        try:
            response = client.service.Collect(policy='logtest001',
                                              provider='bankid',
                                              displayName='Test av Mobilt BankID',
                                              transactionId=transaction_id,
                                              orderRef=seller_agreement.order_ref)
            print response
            if response.progressStatus != val:
                trigger_new_sign_event(seller_agreement.user.id, response.progressStatus)
                val = response.progressStatus
            if (response.progressStatus == 'OUTSTANDING_TRANSACTION' or response.progressStatus == 'USER_SIGN' or response.progressStatus == 'NO_CLIENT'):
                # Try again in 3 seconds
                time.sleep(3)
                continue
            if response.progressStatus == 'COMPLETE':
                # TODO: Save to db
                print "Done!"
            break
        except WebFault, f:
            print f  # Django log?
            trigger_new_sign_event(seller_agreement.user.id, f.fault.faultstring)
            # TODO: cancelled_at?
            break

Answer 1

This docstring is very long and confusing. The official Python style guide recommends that you have a short one line summary of what the function does before launching into longer, detailed paragraphs.

def collect_request(transaction_id):
    '''Waits while collecting a mobile bank ID request.

    You can make a delay for 9 seconds before you start to call the Collect
    method. After 9s you begin to make CollectRequest with a 3 seconds delay
    between each collect. First answer will probably be
    OUTSTANDING_TRANSACTION, then USER_SIGN and at last COMPLETE. (After six
    OUTSTANDING_TRANSACTION you get NO_CLIENT, just to indicate that the user
    not yet has started her BankID client.

    After 180 seconds, you will at last get faultStatus EXPIRED_TRANSACTION'''

I'm not sure I got it quite right, so I'm sure you can come up with a better one. Also there's not much need for using url as a variable directly before it's used 'when you can pass it directly to Client. Though it should either be a constant defined elsewhere or a user provided parameter. There's a lot of values like that in here. Making something a constant makes it clearer what the value means, where it comes from and what it's for.

WSDL_URL = "some url"
PROVIDER = "bankid"

etc.

Your third if should actually be an elif, since the second and third conditions are mutually exclusive. You should then remove the continue and instead manually call break from inside your if response.progressStatus == 'COMPLETE' block. It makes more sense that way, that your loop breaks when this condition is fulfilled, rather than the continue being called in certain other cases. Unless you want to break even if any unhandled progressStatus value is received. ie. As it stands, if progressStatus is anything but 'OUTSTANDING_TRANSACTION', 'USER_SIGN' or 'NO_CLIENT' then the loop breaks. If that is intentional, then you should comment as such.

Also don't use the old syntax for exceptions, it's ambiguous and explicitly doesn't work in Python 3. You should use except WebFault as f. The reason it's ambiguous is that it doesn't allow you to do multiple exceptions properly, imagine you had to try do except WebFault, ValueError, f. It wouldn't make any sense, however you could do except (WebFault, ValueError) as f perfectly fine.

Answer 2

Yes, this code is nicely readable.

This condition can be simplified:

if (response.progressStatus == 'OUTSTANDING_TRANSACTION' or response.progressStatus == 'USER_SIGN' or response.progressStatus == 'NO_CLIENT'):

By using a tuple of the states:

status_to_try_again = ('OUTSTANDING_TRANSACTION', 'USER_SIGN', 'NO_CLIENT')
if response.progressStatus in status_to_try_again:

It might be a good idea to move these string literals to constants at the top of the file.