openshift / ansible-service-broker

Ansible Service Broker
Apache License 2.0
227 stars 84 forks source link

need a way to conditionally render fields #859

Closed jmrodri closed 6 years ago

jmrodri commented 6 years ago

Feature: hide some fields from the Service Bundle from the UI

As seen by the submission of PR #834, there is a desire to have the ability to specify when a parameter should be displayed based on some condition or set of conditions. There is also a desire to have a more robust parameter set in the apb.yml to include objects etc. INSERT FOLLOW ISSUE HERE. This conditional feature needs to be included in the larger parameter discussion.

maleck13 commented 6 years ago

@jmrodri There is a strong need for this feature from many of the mobile APBs being created. I would be happy to take this and progress it in the following way:

Does this suit you or would your prefer to progress yourself?

jwmatthews commented 6 years ago

@maleck13 this sounds like a great feature to get into the Broker/APB spec.

One note of caution, the openshift webui will be moving to React in the future...unsure on timeline, expect it to be within the year..so maybe 6-12 months out. Therefore the implementation that works in angular for short term will need to be updated to React once that happens....just wanted you to be aware if you take this implementation on.

maleck13 commented 6 years ago

@jwmatthews Yes aware of that change coming, but thanks for calling it out. So unless @jmrodri has something in progress or want to take it on, I will start progressing this next week

maleck13 commented 6 years ago

Just to clarify, I would work on a proposal for the the APB spec and any changes needed in the broker, with the intention that we may potentially want to get a change made to the OSB spec at some point in the future once we understand the problem better. I will also reach out to the UX team to start discussions on how this might work from a UI perspective.

jwmatthews commented 6 years ago

@maleck13 writing up a proposal/approach first is the right way to go.

Even a simple google document would help to get an idea on the approach you see and ensure it's feasible.

@tsanders-rh and I spoke to @spadgett ~week ago about some other WebUI changes with JSON schema. The impression I got was JSON schema doesn't lend itself well to presentation information so seemed like our options were to reuse behavior angular-schema-form gives us or to continue with the form data we build up in broker.

In general investigating approaches for richer webui is something we'd love to collaborate on and improve.

If you would, please loop me in on discussions with this.

maleck13 commented 6 years ago

ping @johnfriz

jmrodri commented 6 years ago

@maleck13 I am not actively working on this problem. Like @jwmatthews, it is definitely something that we are interested in. The problem stems from how much UI specific stuff do we put into the broker. It already produces JSON Schema of the parameters, but we also put out an angular-schema-form which apparently is what is used by the WebUI today. The PR to add displayWhen was also adding more to that angular-schema-form which we thought wasn't quite the right solution since it felt odd to have such a specific form being produced by the broker.

Based on @jwmatthews comment about JSON schema not being enough, it sounds like the angular-schema-form is the only solution but just feels odd.

maleck13 commented 6 years ago

Ok, thanks for the useful info. I will take some time next week to look into it. If the schema-form is the only solution, it seems we may want to consider reopening the https://github.com/openshift/ansible-service-broker/pull/834 PR which I will also give a try to see are there UI implications or does it "just work" in the OpenShift UI

maleck13 commented 6 years ago

Just to keep ppl informed this week turned out to be very hectic so planning on looking at this next week now

philipgough commented 6 years ago

Got a chance to look at this today so just circling back around. Building on @ruromero's work on #834 and a small change to the catalog ui https://github.com/openshift/origin-web-catalog/pull/675 this already seems to work well. I built an APB and confirmed I could show/hide form fields based on boolean values, string values and "string contains" in text area.

The PR to add displayWhen was also adding more to that angular-schema-form which we thought wasn't quite the right solution since it felt odd to have such a specific form being produced by the broker.

This is definitely correct although the change in #834 is trivial and the form is returned from the broker currently anyway. If the concern here is moving to React, I took a quick look and some of the json schema lib's for react also support this conditional behaviour. react-jsonschema-form is one such example.

maleck13 commented 6 years ago

Based on https://github.com/openshift/origin-web-catalog/pull/675 it looks like we cannot use the form generation approach as @spadgett points out:

we can't use angular-schema-form conditions because they let brokers run arbitrary script code.

philipgough commented 6 years ago

@maleck13 also I don't believe the angular lib used supports the dependency feature of the schema which is unfortunate since this is implemented in the react lib.

maleck13 commented 6 years ago

Just updating here so all are aware. @david-martin @PhilipGough are doing a PoC collaborating with @spadgett to find a way that this can work. There are more details in the linked issue.

@david-martin @PhilipGough could we add some details here of changes we are experimenting with in the broker to ensure we can get feedback from the team

philipgough commented 6 years ago

@maleck13 not too much to add as of yet. To summarise we are currently investigating if the "dependencies" feature from JSON Schema will fit the needs of our use case. When/if we can confirm that it does, we can start to make some changes in the broker and I will update here. If it doesn't we will need to investigate and propose a custom solution.

philipgough commented 6 years ago

Hi all, based on the conversation with @spadgett and @david-martin on https://github.com/openshift/origin-web-catalog/pull/675 I have done some more investigation into how this solution might look

The keywords to allow the dynamic forms in the react library we are discussing in the linked pr which is now closed , specifically the anyOf keyword, are in fact part of the JSON schema spec. See reference here.

However there have been numerous failed attempts at implementing these keywords in a way that was acceptable in the library. See docs FAQ .

So it seems like everything we would need is in the schema but hasn't made it to the lib which will most likely be used in the catalog.

@maleck13 @david-martin Maybe we could spend some time trying to flesh out an approach based on our current use cases to get the support we will need going forward from the point of view of the broker? So my suggestion is as follows:

What do others think of this potential approach?

david-martin commented 6 years ago

The keywords to allow the dynamic forms in the react library we are discussing in the linked pr which is now closed , specifically the anyOf keyword, are in fact part of the JSON schema spec. See reference here.

Although they are part of the schema, I don't see if they are allowed in the context of dependencies by the schema?

Gather a set of requirements and use cases from others involved here and see what the community needs now as a minimum

A select with dependencies based on the selected option would satisfy all our use cases at this time that I'm aware of.

Investigate how far away the existing code in the react library is from allowing us to provide this

The dynamic dependencies example with oneOf used under dependencies looks like what's needed for our use case. https://github.com/mozilla-services/react-jsonschema-form#dynamic

Assuming the lib is workable, or we feel that we can make the required changes we could allow the user to define the dependency schema within the apb. I feel this is reasonable since it is part of the spec but if its seen as too complex from a dev perspective we could attempt to simplify it and provide some mapping.

Agreed. Lets get a working example and review how complex it is then. The more important thing IMO is where the dependencies object is added to the plan response from the Broker API. i.e. in the plan schema or in the openshift_form_definition. I asked this here https://github.com/openshift/origin-web-catalog/pull/675#issuecomment-384577966

Intermediately, we would need to provide a mechanism to translate a dependency schema to angular conditions and this work would need to go directly into the web-catalog for security reasons

It could be added to the origin-web-catalog directly, or we could write an angular-schema-form add-on that adds dependencies support

philipgough commented 6 years ago

@david-martin

Although they are part of the schema, I don't see if they are allowed in the context of dependencies by the schema?

Dependencies appears to be a map of strings to Schemas so I think this scenario is valid.

If the oneOf keyword is sufficient then I guess we can just do some mapping in the broker to support this and I'll try and progress that.

philipgough commented 6 years ago

So after some discussion with both @david-martin and @spadgett as to what we can get into the current web-catalog and what will work going forward we would like to suggest the following solution: When a user wishes to conditionally render some parameter field in provision and bind form they can define a dependency on the parameter allowing them to achieve this. Example yaml

plans:
  - name: example
    parameters:
    - name: eg_enum_param
      type: enum
      title: Conditional Example
      default: "Yes"
      enum: ["Yes", "No"]
    - name: eg_conditional_one
       title: Example Shown By Default
       dependencies:
         - key: eg_enum_param
            value: "Yes"
    - name: eg_conditional_two
        title: Example Hidden By Default
        dependencies:
          - key: eg_enum_param
             value: "No"

The changes in the previous three referenced pull requests will allow the user to essentially get from this simple definition to https://github.com/mozilla-services/react-jsonschema-form#dynamic which is what the UI team want to use as it is supported by json schema. Changes by @david-martin to catalog will give us this functionality using angular conditions in the process of moving to react.

Anyone have any opinions or concerns on the above?

ruromero commented 6 years ago

@PhilipGough @david-martin I'll add a use case that would be very useful for the project we're working on. It requires support of:

plans:
  - name: example
    parameters:
    - name: database_type
      type: enum
      enum: ["H2", "MySQL", "PostgreSQL", "MariaDB"]
      title: Database Type
    - name: external_database
      type: boolean
      display_type: checkbox
      title: External Database
      dependencies:
        - key: database_type
          value: ["MySQL", "PostgreSQL", "MariaDB"]
    - name: database_url
      title: External Database URL
      type: string
      dependencies:
        - key: external_database
          value: true
    - name: use_persistence
      type: boolean
      display_type: checkbox
      title: Use Persistence
      dependencies:
        - key: external_database
          value: false
    - name: db_volume_size
      title: Database Size
      dependencies:
        - key: use_persistence
          value: true
        - key: external_database
          value: false
david-martin commented 6 years ago

Thanks for the use case @ruromero. I'll try work this into the catalog pr

philipgough commented 6 years ago

Hey @ruromero thanks for the feedback. @david-martin doesn't the above use case then require the use of the allOf keyword in the case of multiple dependencies to ensure the schema is valid, that is if I am not misunderstanding the json schema spec, but this is the point I was making earlier. I'm not sure if the react library has everything we would need currently to support this?

david-martin commented 6 years ago

@PhilipGough Are the non enum examples above not covered by the 'conditionals' part of react-jsonschema-form https://github.com/mozilla-services/react-jsonschema-form#conditional i.e. not oneOf/allOf/anyOf, but just plain dependencies

philipgough commented 6 years ago

@david-martin I think those are (although we will need to test that is the case), the issue I mainly see not covered with @ruromero use case is the multiple dependencies

    - name: db_volume_size
      title: Database Size
      dependencies:
        - key: use_persistence
          value: true
        - key: external_database
          value: false
david-martin commented 6 years ago

@PhilipGough Actually I'm not sure any of that schema is possible with just conditional dependencies, so ignore my last comment. The conditional dependencies just allow for a value being defined, as opposed to having a specific value. So oneOf may allow for most of that schema (but would need to test like you said).

And allOf would need to be added for both angular & react for the one you highlighted. That would be a separate PR for sure.

philipgough commented 6 years ago

The conditional dependencies just allow for a value being defined, as opposed to having a specific value.

I believe it will work correctly for type boolean and the others to just be defined or not

maleck13 commented 6 years ago

@jmrodri @eriknelson @rthallisey Will add to the agenda for next weeks broker meeting. As we have an end to end working sample now. Would be good to get your thoughts on this before we go any further. See demo of it working in the catalog. https://www.youtube.com/watch?v=MX-G0TjaP7E&feature=youtu.be

mhrivnak commented 6 years ago

In order to accomplish this, which draft version of the JSON schema is the minimum required?

The OSBAPI requires the "Platform" to support at least version 4. If any of this depends on a newer draft version, that has to be declared by the broker explicitly in the catalog response.

philipgough commented 6 years ago

@mhrivnak these schema properties are supported as of version 4 see https://tools.ietf.org/html/draft-fge-json-schema-validation-00