Feature json schema #2664

hamzahamidi · 2018-07-11T10:00:01Z

Description

ADD JSON schema form generator to service binding & instance

Motivation and Context

JSON Schema sent by the broker creator is used to generate forms which are used to generate the json parameters for the service binding & service instance.
This feature uses Angular 6 JSON Schema Form builder which generates forms from JSON schemas using Angular.
The library is compatible with version 4 of the JSON schema standard.

Types of changes

Bug fix (non-breaking change which fixes an issue)
Docs update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have followed the guidelines in CONTRIBUTING.md, including the required formatting of the commit message

cfdreddbot · 2018-07-11T10:00:02Z

Hey hamzahamidi!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

codecov · 2018-07-11T10:10:52Z

Codecov Report

Merging #2664 into v2-master will decrease coverage by 21.23%.
The diff coverage is 48.89%.

@@              Coverage Diff               @@
##           v2-master    #2664       +/-   ##
==============================================
- Coverage      70.03%   48.79%   -21.24%     
==============================================
  Files            599      604        +5     
  Lines          25504    25892      +388     
  Branches        5765     5862       +97     
==============================================
- Hits           17862    12635     -5227     
- Misses          7642    13257     +5615

richard-cox · 2018-07-16T10:34:39Z

Hi @hamzahamidi thanks for submitting this PR. Noticed that it currently uses a personal fork of the json schema form field project and that you've created a PR to get this merged back to origin (dschnelldavis/angular2-json-schema-form#295). Do you have a rough idea of when that might happen?

hamzahamidi · 2018-07-16T12:15:19Z

@richard-cox Thank you for your review. We tweaked the owner repo for angular2-json-schema-form & we're still waiting for the merge. This PR is still on progress. We added the JSON schema support for create schemas & we plan to do the same thing to update schemas

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

hamzahamidi · 2018-07-25T09:36:15Z

@richard-cox This PR is ready for review. We switched the personal fork to angular6-json-schema-form because we faced an issue with enabling AOT in the old one.
Thank you.

KlapTrap · 2018-08-02T08:49:02Z

Thanks, we will review this ASAP.

hamzahamidi · 2018-08-07T08:20:45Z

status on the json-schema-form library
original author (@dschnelldavis) maintains the library by batch, and may take time to support merge angular6 and AOT support. See https://github.com/dschnelldavis/angular2-json-schema-form#contributions-and-future-development

If you like this library, need help, or want to contribute, let me know. I'm busy, so it sometimes takes me a long time to respond, but I will eventually get back to you. :-)

I initially proposed dschnelldavis/angular2-json-schema-form#295.
However, this PR did not include AOT support that required deeper changes, I created a hamzahamidi/Angular6-json-schema-form repo
This repo contains additional refactorings over dschnelldavis/angular2-json-schema-form
the plan is indeed to merge back effort with @dschnelldavis and the associated community. This convergence effort in underway into dschnelldavis/angular2-json-schema-form#302
I plan to maintain this new repo until the convergence with @dschnelldavis and the community is finalized
Note that my contract with Orange ends of august and that Orange has not yet identified engineering resources to contribute to stratos yet. If you have a way to priorize review and merging of this PR in the coming days, that will much help me.

richard-cox

We apologise for the amount of time it's taken us to start reviewing this PR. Personally I was hoping that the angular2 forms repo would merge your changes, but appreciate you're doing all you can get your changes merged and that your fork is getting some great traction. It's really good to see it already has multiple contributors and stars. I have a question regarding the angular6-json-schema-form's npm entry though, have all people that have update access to it taken steps to secure their account (such as enabling two-step authentication)?

I've had a quick look through using the schema from https://hamidihamza.com/Angular6-json-schema-form/. There's mostly 'best practise' like comments, nothing major. In addition...

Adding text to fields in the generated form correctly updates the JSON parameters text above, however the reverse does not happen.
When the form is invalid (for example a require field such as last_name is missing) the step is still valid (stepper next button is enabled).
There are some errors shown at the bottom. It feels like these should only be generic issues that are not associated with a specific field, else those errors should appear underneath the fields themselves. For example required field errors appear both beneath the field and in the list at the bottom. For a 'phone number' section a missing number is only shown in the list at the bottom, for example

should have required property 'last_name'
undefined.phone_numbers[0]: should have required property 'number'

The error list at the bottom needs to be more material design like, though this is something we can look at after this PR has merged.
The SpecifyDetailsStep throws the exception below. These are a real pain and basically indicate a child component is changing the state of a parent component. I captured some notes on it a while back https://github.com/cloudfoundry-incubator/stratos/blob/7180ced053e22ad1915c50543d739bf7928f69ac/docs/developers-guide-env-tech.md#expressionchangedafterithasbeencheckederror-error. If you'd like further help on this just let us know.

specify-details-step.component.html:27 ERROR Error: ExpressionChangedAfterItHasBeenCheckedError: Expression has changed after it was checked. Previous value: 'mat-form-field-should-float: false'. Current value: 'mat-form-field-should-float: true'.

There's a new class that's being applied to the mat-form-field-infix divs (see below) which breaks the width of fields in the list component (these are fixed width, but are ignored in favour of this new class). This might disappear when/if '@angular/flex-layout' goes?

mat-form-field .mat-form-field-wrapper .mat-form-field-flex .mat-form-field-infix {
    width: initial;
}

Overall .. some things to work through but it's looking really good!

richard-cox · 2018-08-07T10:25:40Z

package.json

@@ -46,6 +46,7 @@
    "@angular/common": "6.0.3",
    "@angular/compiler": "6.0.3",
    "@angular/core": "6.0.3",
+    "@angular/flex-layout": "^6.0.0-beta.16",


It doesn't look like this is used within Stratos, however removing results in the following error

ERROR in ../../angular6-json-schema-form/angular6-json-schema-form.ts(38,53): Error during template compile of 'MaterialDesignFrameworkModule' Could not resolve @angular/flex-layout relative to /home/richard/dev/github/stratos-ui-orange-cloudfoundry/node_modules/angular6-json-schema-form/angular6-json-schema-form.d.ts..

Should this be a dependency of angular6-json-schema-form?

Yes the material design module uses @angular/flex-layout. I'm open to better approachs

src/frontend/app/core/cf-api-svc.types.ts

richard-cox · 2018-08-07T10:43:20Z

src/frontend/app/features/service-catalog/services-helper.ts

+    }));
+};
+
+export const prettyValidationErrors = (formValidationErrors) => {


formValidationErrors needs a type, that would open up the easier formValidationErrors.forEach iteration function. If you're feeling fancy you could even use formValidationErrors.reduce instead, which would both iterate over formValidationErrors and output the errorArray

richard-cox · 2018-08-07T12:09:31Z

...tend/app/shared/components/add-service-instance/bind-apps-step/bind-apps-step.component.html

+      <mat-card-content>
+        <json-schema-form loadExternalAssets="true" [options]="jsonFormOptions" [schema]="schema" [framework]="selectedFramework" (validationErrors)="validationErrors($event)" (onChanges)="onFormChange($event)">
+        </json-schema-form>
+        <div *ngIf="!!prettyValidationErrors" class="data-bad" [innerHTML]="prettyValidationErrors"></div>


For class names we use BEM notation (for an intro see http://getbem.com/introduction/). At the moment we have BEM checking turned off in our pipeline, but at some point in the future we'd like to turn it back on again.

richard-cox · 2018-08-07T12:10:25Z

...ontend/app/shared/components/add-service-instance/bind-apps-step/bind-apps-step.component.ts

@@ -35,6 +36,14 @@ export class BindAppsStepComponent implements OnDestroy, AfterContentInit {
  stepperForm: FormGroup;
  apps$: Observable<APIResource<IApp>[]>;
  guideText = 'Specify the application to bind (Optional)';
+
+  selectedFramework = 'material-design';
+  schema: any;


If possible it would be nice to type the fields and function params in this class.

richard-cox · 2018-08-07T12:23:31Z

...red/components/add-service-instance/specify-details-step/specify-details-step.component.html

  </form>
+  <div *ngIf="!!schema" class="json-schema">


There's a fair bit of duplication between this step and bind app (html, input, error handling, etc). Could the common parts be pulled out into a new component ServiceBindingForm? This would remove a lot of duplicated html, plumming in the component's code and i think the possibly the need for the scss mixins

richard-cox · 2018-08-07T12:25:10Z

...hared/components/add-service-instance/specify-details-step/specify-details-step.component.ts

-  onEnter = () => {
+  onEnter = (selectedService$?) => {
+    if (selectedService$ instanceof Observable) {
+      this.selectedServiceSubscription = selectedService$


See above comment about using first() or changing to this.schema$ observable

richard-cox · 2018-08-07T12:47:10Z

...ontend/app/shared/components/add-service-instance/bind-apps-step/bind-apps-step.component.ts

+    if (selectedService$ instanceof Observable) {
+      this.selectedServiceSubscription = selectedService$
+        .subscribe(selectedService => {
+          this.schema = this.filterSchema(selectedService.entity.entity.schemas.service_binding.create.parameters);


In the two places this is used need to check whether if schemas has a value (currently gives undefined exception for services without schemas`

Full exception: Cannot read property 'service_binding' of undefined

richard-cox · 2018-08-07T12:51:22Z

...tend/app/shared/components/add-service-instance/bind-apps-step/bind-apps-step.component.html

+  <div *ngIf="!!schema" class="json-schema">
+    <mat-card class="json-schema-form">
+      <mat-card-header>
+        <mat-card-title><b>Generated Form</b><button mat-button color="accent" (click)="showJsonSchema=!showJsonSchema">Json schema</button>


The button needs some padding-left, to ensure the 'on hover' button background has space between itself and text. Should also consider whether this button is something we should hide in production. @KlapTrap thoughts?

richard-cox · 2018-08-07T13:17:04Z

...hared/components/add-service-instance/specify-details-step/specify-details-step.component.ts

+    if (selectedService$ instanceof Observable) {
+      this.selectedServiceSubscription = selectedService$
+        .subscribe(selectedService => {
+          if (!!this.modeService.isEditServiceInstanceMode()) {


If we're in edit mode, should the update section be used?

gberche-orange · 2018-08-08T16:08:20Z

thanks @richard-cox and @KlapTrap for your review

Adding text to fields in the generated form correctly updates the JSON parameters text above, however the reverse does not happen.

Yes, this two way data binding would be quite nice UX. The related work can also for improve the update service instance/binding parameters by prepopulating the form with the params previously submitted by the user. These params values are provided by the new OSB 2.14 GET endpoints for fetching a Service Instance and Service Binding which is now exposed by CC API in the related service instance and service binding endpoints. Related stratos issue: #2121

@hamzahamidi started looking into this but could not yet complete this. This seems possible from
https://github.com/dschnelldavis/angular2-json-schema-form#additional-inputs-an-outputs
to specify the initial (data) values

<json-schema-form
  [schema]="yourJsonSchema"
  [layout]="yourJsonFormLayout"
  [(data)]="yourData"
...
</json-schema-form>

Initial study indicates that real-time two way binding might be hard to achieve. An alternative might be to offer users to either edit in plain text or form, possibly by switching tabs (see mock below that may still include a real-time read-only preview)

I'll let him provide more details, confirm feasibility and required work.

There are some errors shown at the bottom. It feels like these should only be generic issues that are not associated with a specific field, else those errors should appear underneath the fields themselves. For example required field errors appear both beneath the field and in the list at the bottom. For a 'phone number' section a missing number is only shown in the list at the bottom, for example

The errors at the bottom are global validation errors that the library can't easily associate to a single particular field. Common example are violations to attribute dependencies, or missing attributes (i.e. for which associated form entry is not yet displayed, as they are optional). Display of these errors is necessary for users to understand schema constraints violations (as field-attached validation messages would be insufficient).

KlapTrap · 2018-08-08T18:42:01Z

Thanks for your input, I like the mock design, would love to see it in action.

We don’t mind picking up we’re you left off if you don’t quite get it finished before you’re off, you’ve already saved us a lot of time!

gberche-orange · 2018-08-08T22:20:59Z

When the form is invalid (for example a require field such as last_name is missing) the step is still valid (stepper next button is enabled)

We had mixed feelings about systematically disabling the next button on validation violations (currently stratos allows params violating their schema), and rather imagined either a way to have users bypass validation violations if needed, through either:

the "Next" button to change its color (e.g. to red) and text to "Next (ignore schema violations)" on schema violations
Two buttons (possibly stacked): "Next" and "Next (ignore schema violations)" with "Next" being disabled on schema violations

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

richard-cox · 2018-08-20T08:58:13Z

@gberche-orange Mocked functionality looks good. This might also help with the form validation + next issue, (next state tied to form state on form tab, whilst ignored on json tab).
@hamzahamidi I know your time is short and would love to understand any recent progress.

gberche-orange · 2018-08-21T11:39:19Z

@richard-cox

Mocked functionality looks good. This might also help with the form validation + next issue, (next state tied to form state on form tab, whilst ignored on json tab).

From a user perspective, it would be useful to also see validation messages when editing raw json, something like the mock below. Maybe the form could be updated and generate the validation message when focus is lost from the json text, and/or when an explicit refresh button is clicked.

I know your time is short and would love to understand any recent progress.

Hamza is out of office (with limited connectivity to provide status on the last pushed commits) until next monday aug 27th, (he will be holding his internship dissertation defense on that week, so he'll have limited contribution capacity).

richard-cox · 2018-10-02T16:49:37Z

Closing in favour of updated #3050

hamzahamidi force-pushed the feature-json-schema branch from 1d08ff7 to 8ca4612 Compare July 11, 2018 13:04

nwmac added the in progress label Jul 17, 2018

hamzahamidi force-pushed the feature-json-schema branch 3 times, most recently from c9e101d to 8283be4 Compare July 20, 2018 08:21

hamzahamidi added 9 commits July 23, 2018 09:44

ADD angular2-json-schema-form

f2bac91

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

ADD json schema form generator to service binding & instance

d5caf5e

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Fix codeclimate issues

7d746b0

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Fix Error subscribe of undefined onEnter

ebdfcf2

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

ADD mixin for json-schema-component

2603bdc

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Update library Angular 2 JSON Schema Form builder

0879bd0

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Delete unused types

5e6f35a

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Add JSON schema support for update schemas

5b592bb

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

Refractoring

e7c63bf

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

hamzahamidi force-pushed the feature-json-schema branch 2 times, most recently from 3f9ae8a to a448786 Compare July 25, 2018 08:47

Fix AOT build

a448786

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

gberche-orange mentioned this pull request Aug 2, 2018

Json schema support for service instance/binding #1434

Closed

richard-cox added ready for review and removed in progress labels Aug 2, 2018

richard-cox requested review from KlapTrap and richard-cox August 7, 2018 09:59

richard-cox added the in review label Aug 7, 2018

richard-cox suggested changes Aug 7, 2018

View reviewed changes

richard-cox added the needs attention This PR needs attention label Aug 7, 2018

KlapTrap removed in review ready for review labels Aug 8, 2018

hamzahamidi added 3 commits August 9, 2018 17:10

ADD schemaplan types

e4f032c

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

optimize prettyValidationErrors

048bfd9

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

fix Onformchange

f2075b4

Signed-off-by: Hamza Hamidi <hamza.hamidi@orange.com>

richard-cox mentioned this pull request Sep 14, 2018

Feature json schema #2997

Merged

richard-cox mentioned this pull request Sep 21, 2018

Feature json schema #3050

Merged

richard-cox closed this Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature json schema #2664

Feature json schema #2664

hamzahamidi commented Jul 11, 2018 •

edited

Loading

cfdreddbot commented Jul 11, 2018

codecov bot commented Jul 11, 2018 •

edited

Loading

richard-cox commented Jul 16, 2018

hamzahamidi commented Jul 16, 2018 •

edited

Loading

hamzahamidi commented Jul 25, 2018 •

edited

Loading

KlapTrap commented Aug 2, 2018

hamzahamidi commented Aug 7, 2018

richard-cox left a comment

richard-cox Aug 7, 2018

hamzahamidi Aug 8, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

KlapTrap Aug 7, 2018

richard-cox Aug 7, 2018

richard-cox Aug 7, 2018

gberche-orange commented Aug 8, 2018 •

edited

Loading

KlapTrap commented Aug 8, 2018

gberche-orange commented Aug 8, 2018 •

edited

Loading

richard-cox commented Aug 20, 2018

gberche-orange commented Aug 21, 2018

richard-cox commented Oct 2, 2018

Feature json schema #2664

Feature json schema #2664

Conversation

hamzahamidi commented Jul 11, 2018 • edited Loading

Description

Motivation and Context

Types of changes

Checklist:

cfdreddbot commented Jul 11, 2018

codecov bot commented Jul 11, 2018 • edited Loading

Codecov Report

richard-cox commented Jul 16, 2018

hamzahamidi commented Jul 16, 2018 • edited Loading

hamzahamidi commented Jul 25, 2018 • edited Loading

KlapTrap commented Aug 2, 2018

hamzahamidi commented Aug 7, 2018

richard-cox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gberche-orange commented Aug 8, 2018 • edited Loading

KlapTrap commented Aug 8, 2018

gberche-orange commented Aug 8, 2018 • edited Loading

richard-cox commented Aug 20, 2018

gberche-orange commented Aug 21, 2018

richard-cox commented Oct 2, 2018

hamzahamidi commented Jul 11, 2018 •

edited

Loading

codecov bot commented Jul 11, 2018 •

edited

Loading

hamzahamidi commented Jul 16, 2018 •

edited

Loading

hamzahamidi commented Jul 25, 2018 •

edited

Loading

gberche-orange commented Aug 8, 2018 •

edited

Loading

gberche-orange commented Aug 8, 2018 •

edited

Loading