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.

Sign up for GitHub

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

Jump to bottom

Add an option to mark that the Service acts as product (THREESCALE-3294) #1119

Merged
merged 1 commit into from Aug 29, 2019
Merged

Add an option to mark that the Service acts as product (THREESCALE-3294) #1119

merged 1 commit into from Aug 29, 2019

Conversation

duduribeiro
Copy link
Contributor

@duduribeiro duduribeiro commented Aug 22, 2019 •
edited

What this PR does / why we need it:

This commit adds a new option in the screen to create a service
to inform if it will act as product. If this options is checked, it will
present a select to choose which backend api it will be attached
or if it will create a new one.

This is when you create a Service without checking act as product:
act_as_product_1

This is when you create a Service checking act as product and let it create a new Backend API:
act_as_product_2

This is when you create a Service checking act as product and select an already existent Backend API:
act_as_product_3

Which issue(s) this PR fixes

THREESCALE-3294

Verification steps

  1. Go to the screen to create a new Service
  2. Create a new service without checking act as product checkbox.
  3. It should create a new service without attaching any backend api into it.
  4. Go to the screen to create a new Service
  5. Create a new service checking act as product checkbox and selecting New Backend API in the Backend API combo box.
  6. It should create a new service with a freshly created backend api for this service.
  7. Go to the screen to create a new Service
  8. Create a new service checking act as product checkbox and selecting some already created backend api in the Backend API combo box.
  9. It should create a new service with an already existent backend api for this service.

@duduribeiro duduribeiro self-assigned this Aug 22, 2019
@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch 2 times, most recently from 9c51cbf to 8f7b5db Compare August 22, 2019 21:15
Martouta
Martouta reviewed Aug 22, 2019
View reviewed changes
app/controllers/api/services_controller.rb Outdated
private

def find_backend_api
current_account.backend_apis.find(params[:service][:backend_api])
Copy link
Contributor

Choose a reason for hiding this comment

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

current_account.backend_apis.find(params.require(:service).require(:backend_api]) because otherwise if service is not in the params, it will raise NoMethodError

Martouta
Martouta reviewed Aug 22, 2019
View reviewed changes
app/controllers/api/services_controller.rb Outdated

def service_params
params.require(:service).permit(:system_name, :name, :description, :act_as_product).tap do |p|
p[:backend_api_configs] = [ BackendApiConfig.new(backend_api: find_backend_api) ] if params[:service][:backend_api].present?
Copy link
Contributor

Choose a reason for hiding this comment

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

only do this is the account is authorized to do it 😄 or if we are not doing the authorizations in depth yet, then at leaaast make sure the rolling update is enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

and add a test to make sure this backend api part works and only if the rolling update is enabled

@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch from 8f7b5db to 6d07d6e Compare August 22, 2019 22:21
josemigallas
josemigallas requested changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

SelectBackendApi should be rendered in NewServiceForm instead of inside both Service*Form.

app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated
import {Label} from 'NewService/components/FormElements'

const SelectBackendApi = ({backendApis}: {backendApis: Array<{id: number, name: string}>}) => {
const [actAsProduct, setActAsProduct] = useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Flow complaints about this state not being covered, you can give it a type like this: useState<boolean>(false)

Note: in the past this would cause an error in Jest, let's keep an eye on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any error in Flow.
Where do you see it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see error on this too.

app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated Show resolved Hide resolved
spec/javascripts/NewService/NewServiceForm.spec.jsx Outdated Show resolved Hide resolved
spec/javascripts/NewService/NewServiceForm.spec.jsx Outdated Show resolved Hide resolved
spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/NewServiceForm.jsx Outdated Show resolved Hide resolved
spec/javascripts/NewService/NewServiceForm.spec.jsx Show resolved Hide resolved
damianpm
damianpm suggested changes Aug 23, 2019
View reviewed changes
app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated
import {Label} from 'NewService/components/FormElements'

const SelectBackendApi = ({backendApis}: {backendApis: Array<{id: number, name: string}>}) => {
const [actAsProduct, setActAsProduct] = useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any error in Flow.
Where do you see it?

app/javascript/src/NewService/components/FormElements/SelectBackendApi.jsx Outdated
htmlFor='service_backend_api'
label='Backend API'
/>
<select name="service[backend_api]" id="service_backend_api">
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have a select component you can reuse in
app/javascript/src/NewService/components/FormElements/Select.jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't use it because that Select relies to set the value of option the same as the text. I want the id as the value and the name as the text

app/javascript/src/NewService/components/FormElements/ServiceDiscoveryListItems.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/FormElements/ServiceManualListItems.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/NewServiceForm.jsx Show resolved Hide resolved
app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx Outdated Show resolved Hide resolved
app/javascript/src/NewService/components/ServiceManualForm.jsx Show resolved Hide resolved
spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx Outdated Show resolved Hide resolved
spec/javascripts/NewService/formElements/ServiceDiscoveryListItems.spec.jsx Outdated Show resolved Hide resolved
@guicassolato
Copy link
Contributor

When an existing Backend API is selected to start the service with (as opposed to using the option "New Backend API"), we need to make sure the existing metrics of that backend API are synced with apisonator together with the new service.

Right now I know we sync new metrics just added to a backend through all services using the backend, but not existing metrics of a backend when the backend is added to a service.

The same needs to be done when we add a backend API to a existing service. (Another pending issue of APIAP, BTW.)

@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch 4 times, most recently from 990f349 to 14e87f0 Compare August 27, 2019 00:09
josemigallas
josemigallas requested changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

I think we should remove ServiceManualForm and ServiceDiscoveryForm from the map and simply use ServiceManualListItems and ServiceDiscoveryListItems respectively.

@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch from 14e87f0 to fc39be7 Compare August 27, 2019 15:11
@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch from fc39be7 to 1d21f49 Compare August 27, 2019 15:16
@thomasmaas
Copy link
Member

So, i guess i was out of the loop on this one. In my head services and api as product were never intended to exist side by side and thus this feature of marking a service as a product wouldn't exist.

hallelujah
hallelujah reviewed Aug 27, 2019
View reviewed changes
app/models/backend_api_config.rb
@@ -11,6 +11,10 @@ class BackendApiConfig < ApplicationRecord
has_one :proxy, through: :service
has_many :proxy_rules, through: :proxy

after_create do
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 part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is. @guicassolato commented that we need to sync the metrics with the backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is because you can choose an existing backend API instead of creating a new one.

@duduribeiro
Copy link
Contributor Author

@thomasmaas @guicassolato @hallelujah so this is not valid ? or we should have this until we do not accept services without api as product?

@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch 2 times, most recently from 48427ed to 0dd700b Compare August 27, 2019 15:36
@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch 4 times, most recently from 41883ef to 92924b2 Compare August 27, 2019 18:20
@duduribeiro
Add an option to mark that the Service acts as product
5ed20fd 
THREESCALE-3294

This commit adds a new option in the screen to create a service to
inform that it will act as product. If this options is checked, it
will present a select to choose which backend api will be attached
or if it will create a new one.
@duduribeiro duduribeiro force-pushed the option_to_mark_that_api_acts_as_product branch from 92924b2 to 5ed20fd Compare August 27, 2019 18:46
damianpm
damianpm approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@damianpm damianpm left a comment •
edited

Choose a reason for hiding this comment

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

@duduribeiro It's OK for me, but let's wait for an approval of a rubyist

guicassolato
guicassolato reviewed Aug 28, 2019
View reviewed changes
app/controllers/api/services_controller.rb
@@ -34,8 +36,8 @@ def settings

def create
@service = collection.new # this is done in 2 steps so that the account_id is in place as preffix_key relies on it
@service.attributes = params[:service]
@service.system_name = params[:service][:system_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did system_name need to be set separately and now we don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't know. I thought this was strange and removed, and it worked .

guicassolato
guicassolato reviewed Aug 28, 2019
View reviewed changes
app/controllers/api/services_controller.rb
def service_params
allowed_params = %i[system_name name description]
allowed_params << :act_as_product if provider_can_use?(:api_as_product)
params.require(:service).permit(allowed_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
params.require(:service).permit(allowed_params)
params.require(:service).permit(*allowed_params)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really needed as Rails flattens the array.

@duduribeiro duduribeiro merged commit 9529edd into master Aug 29, 2019
@duduribeiro duduribeiro deleted the option_to_mark_that_api_acts_as_product branch August 29, 2019 13:50
@secretsurf secretsurf changed the title Add an option to mark that the Service acts as product Add an option to mark that the Service acts as product (THREESCALE-3294) Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Martouta Martouta Martouta left review comments

@guicassolato guicassolato guicassolato approved these changes

@josemigallas josemigallas josemigallas approved these changes

@damianpm damianpm damianpm approved these changes

@hallelujah hallelujah Awaiting requested review from hallelujah

Assignees

@duduribeiro duduribeiro

Labels
APIAP
Projects
None yet
Milestone
No milestone
7 participants
@duduribeiro @guicassolato @thomasmaas @hallelujah @Martouta @josemigallas @damianpm