CheckBillingOrderStatus() is misnamed

[riethm]

helpers/order/order.go:

// CheckBillingOrderStatus returns true if the status of the billing order for
// the provided product order receipt is in the list of provided statuses.
// Returns false otherwise, along with the billing order item used to check the statuses,
// and any error encountered.
func CheckBillingOrderStatus(sess *session.Session, receipt *datatypes.Container_Product_Order_Receipt, statuses []string) (bool, *datatypes.Billing_Order_Item, error) {
    service := services.GetBillingOrderItemService(sess)

    item, err := service.
        Id(*receipt.PlacedOrder.Items[0].Id).
        Mask("mask[id,billingItem[id,provisionTransaction[id,transactionStatus[name]]]]").
        GetObject()

    if err != nil {
        return false, nil, err
    }

    currentStatus := *item.BillingItem.ProvisionTransaction.TransactionStatus.Name
    for _, status := range statuses {
        if currentStatus == status {
            return true, &item, nil
        }
    }

    return false, &item, nil
}

The helper name suggests that the status of a Billing_Order is checked, when in-fact:

• only the first order item found is actually inspected
• it checks the provision transaction, not the order status.

Provision transactions are not applicable to all types of orders (e.g., guest upgrade orders do not have provision transactions, resulting in a panic when passing an upgrade order receipt to this helper.

These helpers should either be renamed to indicate that they are to be used to check for provisioning transactions, or updated to check the actual order status.

If the former approach is adopted, some thought should also be given as to whether it is appropriate to stop and return after checking only one order item out of potentially multiple to an order

[renier]

@riethm For guest upgrade orders, you also need a VM reboot for the changes to take a effect. Is this covered by the true order statuses? If not, how can you really check on the status of a guest upgrade?

My feeling is that when checking the billing order status, what most users will really be interested in is the provision status associated with that billing order. We can certainly change the name if this is causing confusion. I also like the idea of passing the billing order item instead of the receipt, to allow checking on multiple items. However, would like to see an example of this need.

[riethm]

Upgrade orders seem to be different than provisioning orders, in that provisioning orders only reach a final status of 'APPROVED' while upgrades go on to 'COMPLETED'. From what I can tell, once the order status is COMPLETED, the upgrade was complete. But I can hardly ever be certain of that. Also, I did manage to find two upgrade orders in our account with provisioning transactions.

[riethm]

Personally, I'm not sure it's worth trying to capture the ordering/provisioning/status-update process into a general purpose helper. If we want to have a helper that checks the first provisioning transaction in an order, that's fine, but let's name it appropriately so there will be no false pretense. When I placed an upgrade order and passed the receipt to CheckBillingOrderStatus() I expected it would do what it's name implied. It didn't.

Trying to avoid a situation like we had with the previous softlayer Go library, where several functions which should have been general purpose were instead tailored for a single use case. That was the impetus for creating this library.

[riethm]

@renier 1cfd138

Comments?

[renier]

@riethm I don't think this is a case of a single-use case helper function. I do think that your example of a vm upgrade behaves differently from what would generally be expected, and it is not even something you could count on, since it tells you nothing about the reboot, which is part of "provisioning" that order (i.e. SoftLayer does not provide helpful provisioning status on that kind of order), which is what most users would really be interested on.

Should we add more documentation to the function about how it does what it does? What would you add? Also, if you think it should be renamed, what should that new name be? PRs welcome.

[riethm]

@renier "single-use case" may have been too strong. Limited then because it can only be relied on to be accurate when your order has a single order-item only.

My preference would be for one of the following:

1. Check all order items in a receipt and only return true when all of them meet the criteria (but then which one to return becomes ambiguous, and what if some have provisioning transactions and others don't, etc.)
2. Change the signature to accept an order item rather than an entire receipt (as I have attempted in 1cfd138). No ambiguity. Works for single item as well as iteratively checking multiple item orders.

VM upgrade orders are indeed different, and would not use this.