Skip to content

expose report_usage command behavior as restful api#111

Merged
infinitewarp merged 2 commits intomasterfrom
81-report-usage-http-api
Mar 9, 2018
Merged

expose report_usage command behavior as restful api#111
infinitewarp merged 2 commits intomasterfrom
81-report-usage-http-api

Conversation

@infinitewarp
Copy link
Member

implements #81

@infinitewarp infinitewarp requested a review from a user March 8, 2018 22:39
@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #111 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
+ Coverage    99.4%   99.44%   +0.04%     
==========================================
  Files          26       28       +2     
  Lines        1169     1258      +89     
  Branches       73       74       +1     
==========================================
+ Hits         1162     1251      +89     
  Misses          3        3              
  Partials        4        4
Impacted Files Coverage Δ
cloudigrade/account/views.py 100% <100%> (ø) ⬆️
cloudigrade/account/serializers.py 100% <100%> (ø) ⬆️
cloudigrade/account/reports.py 100% <100%> (ø) ⬆️
cloudigrade/account/tests/test_reports.py 100% <100%> (ø)
cloudigrade/account/tests/test_views.py 100% <100%> (ø)
cloudigrade/account/tests/test_serializers.py 100% <100%> (ø) ⬆️
...digrade/account/tests/test_command_report_usage.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 325c55e...b0bb850. Read the comment docs.

@infinitewarp infinitewarp force-pushed the 81-report-usage-http-api branch from 1ff6297 to 3ce49c4 Compare March 9, 2018 14:47
Copy link

Choose a reason for hiding this comment

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

We should probably use Account.objects.filter(account_id=account_id).exists(), it should be a little faster since we are just checking for the presence of the object. Additionally, maybe we could add if not and throw an exception if the account does not exist?

Copy link

Choose a reason for hiding this comment

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

If we are testing serializers, shouldn't this come from account.serializers?

@infinitewarp infinitewarp force-pushed the 81-report-usage-http-api branch from 3ce49c4 to b0bb850 Compare March 9, 2018 20:24

router = routers.DefaultRouter()
router.register(r'account', AccountViewSet)
router.register(r'report', ReportViewSet, base_name='report')
Copy link

Choose a reason for hiding this comment

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

Is there a reason for setting the base_name here? It doesn't look like we defined a custom get_queryset method in the ReportViewSet .

Copy link
Member Author

@infinitewarp infinitewarp Mar 9, 2018

Choose a reason for hiding this comment

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

The base_name is necessary because ReportViewSet doesn't have a custom get_queryset or queryset, and I didn't give it a custom get_queryset because I couldn't think of a meaningful queryset to put there since all the fun model work for reporting happens a few steps away in get_hourly_usage.

Copy link

Choose a reason for hiding this comment

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

@infinitewarp infinitewarp merged commit dbc6f4b into master Mar 9, 2018
@infinitewarp infinitewarp deleted the 81-report-usage-http-api branch March 9, 2018 20:45
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.

1 participant