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

Fix EnvironmentAware#environment_id when building entries or assets #195

Merged
merged 1 commit into from
Jun 12, 2019
Merged

Fix EnvironmentAware#environment_id when building entries or assets #195

merged 1 commit into from
Jun 12, 2019

Conversation

timfjord
Copy link
Contributor

Currently both content_type.entries.new and environment.assets.new(basically ContentTypeEntryMethodsFactory#new and EnvironmentEntryMethodsFactory#new) don't work corrently when enviorment is not master. An object that is returned is always targeted to master

content_type.entries.new case

In this case, enviorment_id doesn't calculate properly, because sys doesn't really have any information about the current environment. But there is content_type reader that has #enviorment_id.

environment.assets.new case

In this case, sys[:enviorment] is present, but it is not Contentful::Management::Link, it is Contentful::Management::Environment.

This PR extends EnvironmentAware#environment_id and adds support for both cases. It might even fix other similar cases

It would be great to cover that with integrations specs. Unfortunately, I don't have access to the testing Contentful account, so I can just draft the specs but they need to be finalised by someone with the access


return env_from_sys if env_from_sys

respond_to?(:content_type) && content_type && content_type.environment_id || 'master'
Copy link
Contributor Author

@timfjord timfjord May 27, 2019

Choose a reason for hiding this comment

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

For some reasons content_type&environment_id raises https://travis-ci.org/contentful/contentful-management.rb/jobs/537759420

@HQ063
Copy link
Contributor

HQ063 commented Jun 11, 2019

This must be merged, is an important fix, the current behaviour is clearly bugged.

@dlitvakb
Copy link
Contributor

Hey @Timsly and @HQ063,

Sorry for the delay, merging and adding tests, will release as soon as possible.

Cheers

@dlitvakb
Copy link
Contributor

Released in 2.10.0

@timfjord
Copy link
Contributor Author

Thanks @dlitvakb 👍

@timfjord timfjord deleted the content-type-env branch June 12, 2019 10: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.

None yet

3 participants