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

need a way to conditionally render fields #859

Closed
jmrodri opened this issue Mar 27, 2018 · 28 comments · Fixed by #928
Closed

need a way to conditionally render fields #859

jmrodri opened this issue Mar 27, 2018 · 28 comments · Fixed by #928
Labels

Comments

@jmrodri
Copy link
Contributor

jmrodri commented Mar 27, 2018

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
Copy link
Contributor

maleck13 commented Apr 4, 2018

@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:

  • As a proposal for changes to the apb.yaml parameter definition
  • Also with the UX team to figure out how to make it work from the UX side.

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

@jwmatthews
Copy link
Member

@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
Copy link
Contributor

maleck13 commented Apr 4, 2018

@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
Copy link
Contributor

maleck13 commented Apr 4, 2018

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
Copy link
Member

@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
Copy link
Contributor

maleck13 commented Apr 5, 2018

ping @johnfriz

@jmrodri
Copy link
Contributor Author

jmrodri commented Apr 5, 2018

@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
Copy link
Contributor

maleck13 commented Apr 6, 2018

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 #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
Copy link
Contributor

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

@philipgough
Copy link
Contributor

philipgough commented Apr 23, 2018

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 openshift/origin-web-catalog#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
Copy link
Contributor

Based on openshift/origin-web-catalog#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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

philipgough commented Apr 27, 2018

Hi all, based on the conversation with @spadgett and @david-martin on openshift/origin-web-catalog#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:

  • Gather a set of requirements and use cases from others involved here and see what the community needs now as a minimum
  • Investigate how far away the existing code in the react library is from allowing us to provide this
  • 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.
  • 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

What do others think of this potential approach?

@david-martin
Copy link
Member

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 openshift/origin-web-catalog#675 (comment)

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
Copy link
Contributor

@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
Copy link
Contributor

philipgough commented May 4, 2018

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
Copy link
Member

ruromero commented May 4, 2018

@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:

  • Any type (boolean, string, int, number)
  • Multiple values
  • Multiple dependencies
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
Copy link
Member

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

@philipgough
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor

maleck13 commented May 9, 2018

@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
Copy link
Member

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants