Skip to content

Conversation

FernandoOjeda
Copy link
Contributor

Fixed vs create with flavor data test #1033

@coveralls
Copy link

coveralls commented Sep 7, 2018

Coverage Status

Coverage increased (+0.2%) to 89.305% when pulling c837331 on FernandoOjeda:fo_vs_create_flavor_test into 86d658b on softlayer:master.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Make sure prices are 3 decimal places long
  • Double check the output to make sure it looks like the pre-receipt in the portal
  • Maybe sort the prices by their cost, so the most expensive are at the top?

preset_id = 405
preset_prices = self.ordering.get_preset_prices(preset_id)

self.assertEqual(preset_prices, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to check that the object returned is exactly what is in the Fixture, that just makes this test a pain to update whenever the fixture is changed.

For manager tests like this, I'd prefer you just test that the appropriate API call was made with assert_called_with

You can use the AssertIn to check that the right keyName was returned though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

total_monthly = 0.0
total_hourly = 0.0
total_preset_monthly = 0.0
total_preset_hourly = 0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need 4 variables to track total cost, lets merge these into 1 variable total_cost, that should simplify the logic here a lot.

Your function get_total_recurring_fee will just output the total amount, and we can check the CLI option to see if the number is an hourly or monthly number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

env.fout(output)


def get_total_recurring_fee(args, result, table, total_hourly, total_monthly):
Copy link
Member

@allmightyspiff allmightyspiff Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like functions that modify the arguments that get passed in, good way to introduce bugs in my opinion.

Lets change this function to _build_reciept_table(prices) which will build and return the whole table used to print costs. It doesn't need to know about hourly v monthly.

def _build_reciept_table(prices, billing="hourly"):
    """Retrieve the total recurring fee of the items prices"""
    total = 0.0
    table = formatting.Table(['Cost', 'Item'])
    table.align['Cost'] = 'r'
    table.align['Item'] = 'l'
    for price in prices:
        rate = 0.0
        if billing == "hourly":
            rate += float(price.get('hourlyRecurringFee', 0.0))
        else:
            rate += float(price.get('recurringFee', 0.0))
        total += rate

        table.add_row(["%.3f" % rate, price['item']['description']])
    table.add_row(["%.3f" % total, "Total %s cost" % billing])
    return table

prices need to be to 3 decimals

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

rate = "%.2f" % float(price['recurringFee'])
if result['presetId']:
ordering_mgr = SoftLayer.OrderingManager(env.client)
preset_prices = ordering_mgr.get_preset_prices(result['presetId'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are ordering a preset, what you should do is get the preset prices, and then add the server and ram items to the results['prices'] array. It looks like the receipt already has the disk prices so no worries there.

if result['presetId']:
    ordering_mgr = SoftLayer.OrderingManager(env.client)
    preset_prices = ordering_mgr.get_preset_prices(result['presetId'])
    search_keys = ["guest_core", "ram"]
    for price in preset_prices['prices']:
        if price['item']['itemCategory']['categoryCode'] in search_keys:
            result['prices'].append(price)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the result data doesn't have the core/ram info is a mystery to me, but that was the best I could think of to get that data but NOT get all the other price data that gets returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@allmightyspiff
Copy link
Member

While testing I made some changes that I think work, your welcome to apply them if it makes it easier
patch.txt

@allmightyspiff
Copy link
Member

allmightyspiff commented Sep 10, 2018

screen shot 2018-09-10 at 6 01 32 pm

The price for RAM+CPU from the API doens't seem to match up with the portal. Looks like our API call to get prices might need to take into account the region as well. The current call only gets the default prices.

@FernandoOjeda
Copy link
Contributor Author

Now the total cost is right.
image

@allmightyspiff allmightyspiff merged commit 7c1a3ea into softlayer:master Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants