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

Unwanted text in product settings RadioGroup option #3030

Merged
merged 6 commits into from Aug 5, 2022

Conversation

lvillen
Copy link
Contributor

@lvillen lvillen commented Jul 29, 2022

Which issue(s) this PR fixes

THREESCALE-8556 Unwanted text in product settings RadioGroup option

Verification steps

  1. Navigate to Product setting page (/apiconfig/services/{product_id}/settings).
  2. Confirm Istio option has only "Istio".

Captura de Pantalla 2022-07-29 a las 17 43 13

Special notes for your reviewer:

After some considerations, I've decided to delete the unwanted text form the en.yml file instead of hiding it. That text was hidden at version 2.7 and has been like that since, so it made more sense to simply delete it.

@lvillen lvillen requested a review from a team July 29, 2022 15:44
@lvillen lvillen self-assigned this Jul 29, 2022
test/integration/api/services_controller_test.rb Outdated Show resolved Hide resolved
@@ -199,6 +218,7 @@ class SettingsTest < self
def update_params(oidc_id: nil)
@update_params ||= { service:
{ intentions_required: '0',
deployment_option: 'service_mesh_istio',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this line I wanted to test that we can actually change the Deployment option, also that the Istio option was available and that everything was working as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is incorrect to set it globally like that. I see tests that expect it to be apicast. I don't know how they pass. But either case, in your test case, you can do:

      put admin_service_path(service), params: update_params(deployment_option: 'service_mesh_istio')

Then check value is what you expect. For example you can do this on line 93 instead of setting it directly.

config/locales/en.yml Show resolved Hide resolved
Copy link
Contributor

@akostadinov akostadinov left a comment

Choose a reason for hiding this comment

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

Changes make sense to me.

@@ -199,6 +218,7 @@ class SettingsTest < self
def update_params(oidc_id: nil)
@update_params ||= { service:
{ intentions_required: '0',
deployment_option: 'service_mesh_istio',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is incorrect to set it globally like that. I see tests that expect it to be apicast. I don't know how they pass. But either case, in your test case, you can do:

      put admin_service_path(service), params: update_params(deployment_option: 'service_mesh_istio')

Then check value is what you expect. For example you can do this on line 93 instead of setting it directly.

@lvillen lvillen merged this pull request into fips Aug 5, 2022
@lvillen lvillen deleted the fix-unwanted-span-at-product-settings-istio branch August 5, 2022 13:54
akostadinov pushed a commit that referenced this pull request Aug 5, 2022
Fix Product > Integration > Settings: Unwanted text in product settings RadioGroup deployment option
akostadinov pushed a commit that referenced this pull request Aug 5, 2022
Fix Product > Integration > Settings: Unwanted text in product settings RadioGroup deployment option
akostadinov pushed a commit that referenced this pull request Aug 8, 2022
Fix Product > Integration > Settings: Unwanted text in product settings RadioGroup deployment option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants