Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render links with compressed ids #15659

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

imtayadeway
Copy link
Contributor

@miq-bot add-label api, enhancement
@miq-bot assign @abellotti

@imtayadeway imtayadeway force-pushed the api/render-hrefs-using-cids branch 2 times, most recently from d4b3b5b to 297ca7a Compare July 31, 2017 17:04
@@ -195,7 +199,7 @@
expect do
run_post("#{service_orders_url("cart")}/service_requests",
:action => :add,
:resources => [{:service_template_href => service_templates_url(service_template.id)}])
Copy link
Member

Choose a reason for hiding this comment

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

should not have been changed, keeping inbound post as is.

@abellotti
Copy link
Member

looks good, the one POST to change to same input, also where specs redefine compressed url's in the expectation, might be nice to declare those similarly to their uncompressed counterparts, i.e. vm1_url and vm1_compress_url.

@imtayadeway imtayadeway force-pushed the api/render-hrefs-using-cids branch from 297ca7a to 71a4054 Compare August 3, 2017 21:45
@abellotti
Copy link
Member

we also need the href_slugs compressed for consistency, I think both methods in lib/api/utils.rb need updating.

@imtayadeway imtayadeway force-pushed the api/render-hrefs-using-cids branch 2 times, most recently from bf9cd15 to 46828d9 Compare August 4, 2017 16:02
@imtayadeway
Copy link
Contributor Author

@abellotti took care of those concerns, but I think the might be nice part I'll leave for a follow up? With a big change like this I think I prefer seeing what changed directly even if it makes things a bit more verbose

@abellotti
Copy link
Member

Thanks @imtayadeway for the update. can I bother you for one additional test in spec/lib/api/utils_spec.rb so we test fetching with resource_search_by_href_slug with both id and compressed id ?

@imtayadeway imtayadeway force-pushed the api/render-hrefs-using-cids branch from 46828d9 to 195530a Compare August 4, 2017 17:12
@miq-bot
Copy link
Member

miq-bot commented Aug 4, 2017

Checked commits imtayadeway/manageiq@4b46873~...195530a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
59 files checked, 2 offenses detected

app/controllers/api/base_controller/renderer.rb

@imtayadeway imtayadeway closed this Aug 4, 2017
@imtayadeway imtayadeway reopened this Aug 4, 2017
@abellotti
Copy link
Member

Thanks @imtayadeway for updating this. 🎼

I'll go ahead and merge this.

Let's sync up you, myself and @jntullo once in the repo regarding the nice part. We can take a single spec example that uses both ways (let defined *_url for requests and generated compressed url for the responses) and find a consistent/elegant signature for both before we update them all. Thanks.

@abellotti abellotti merged commit 6c487b6 into ManageIQ:master Aug 4, 2017
@abellotti abellotti added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants