[Custom Policies] Show full schema info on form preview #712

didierofrivia · 2019-03-18T15:42:01Z

What this PR does / why we need it:

When editing the policy schema, the form was only displaying the configuration, but not the rest of the attributes (name, version, etc)

Which issue(s) this PR fixes

fixes https://issues.jboss.org/browse/THREESCALE-2098

Verification steps

Go to Custom Policies and you now should see the full schema reflected on the preview.

Special notes for your reviewer:

hallelujah · 2019-03-19T08:58:18Z

app/controllers/provider/admin/registry/policies_controller.rb

-      name: '[Name of the policy]',
-      summary: '[A brief description of what it does.]',
-      version: '[0.0.1]',
+      name: 'Name of the policy',


I think it is good to leave those brackets. As we validate those fields, if the user does not edit them they will be invalid and the policy cannot be saved

the apicast manifest fails to validate the version, the regex doesn't expects those brackets.

Yes that is what I am saying, so the customer will not save this by inadvertence.
Or if you can make them as placeholder and not real values?

Thing is the form will fail to render by default. This way it will be shown and communicating the user the right way to do it.

codecov · 2019-03-19T14:36:23Z

Codecov Report

Merging #712 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   92.84%   92.84%   +<.01%     
==========================================
  Files        2387     2388       +1     
  Lines       77549    77579      +30     
==========================================
+ Hits        71999    72030      +31     
+ Misses       5550     5549       -1

Impacted Files	Coverage Δ
...ers/provider/admin/registry/policies_controller.rb	`92.3% <ø> (ø)`	⬆️
app/mailers/mail_preview.rb	`33.73% <0%> (-1.15%)`	⬇️
...ications/new_notification_system_migration_test.rb	`98.33% <0%> (-0.08%)`	⬇️
test/unit/observers/message_observer_test.rb	`100% <0%> (ø)`	⬆️
test/workers/create_service_token_worker_test.rb	`100% <0%> (ø)`	⬆️
app/workers/create_service_token_worker.rb	`100% <0%> (ø)`	⬆️
...n/admin/api/buyers_applications_controller_test.rb	`100% <0%> (ø)`	⬆️
...notifications/new_notification_system_migration.rb	`100% <0%> (ø)`	⬆️
app/observers/message_observer.rb	`100% <0%> (ø)`	⬆️
test/unit/observers/cinstance_observer_test.rb	`100% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc5bced...b3451ea. Read the comment docs.

thomasmaas · 2019-03-19T17:27:32Z

app/javascript/src/Policies/components/CustomPolicy.jsx

-    'summary': '[A brief description of what it does.]',
-    'version': '[0.0.1]',
+    'name': 'Name of the policy',
+    'summary': 'A brief description of what it does.',


Suggested change

'summary': 'A brief description of what it does.',

'summary': 'A one-line summary of what this policy does.',

It should be less than 75 characters. Probably good to add this indication

thomasmaas · 2019-03-19T17:28:00Z

app/javascript/src/Policies/components/CustomPolicy.jsx

-    'version': '[0.0.1]',
+    'name': 'Name of the policy',
+    'summary': 'A brief description of what it does.',
+    'description': 'A description of what it does.',


A complete description of what this policy does.

damianpm · 2019-03-20T11:24:05Z

app/javascript/src/Policies/components/CustomPolicy.jsx

@@ -117,24 +62,35 @@ function CustomPolicyForm ({policy, onChange}: {policy: Policy, onChange: OnChan
  )
 }

-function Form ({initialPolicy}: {initialPolicy: Policy}): React.Node {
+function CustomPolicyEditor ({initialPolicy}: {initialPolicy: Policy}): React.Node {


The return type React.Node is not mandatory. In some files we are using it, in others we aren't. We should agree in using it or not. At a glance, most files don't have it, so we could follow that way, unless you have a reason to explicitly include it.

ok, then less is better

app/javascript/src/Policies/components/SchemaEditor.jsx

app/javascript/src/Policies/styles/policies.scss

spec/javascripts/Policies/components/CustomPolicy.spec.jsx

damianpm · 2019-03-20T13:58:10Z

app/javascript/src/Policies/components/CustomPolicy.jsx

-    </div>
-  )
-}
-
 function CustomPolicyForm ({policy, onChange}: {policy: Policy, onChange: OnChange}): React.Node {


You missed this one React.Node

* Resides on a separate component * Does specific schema validation * Better error communication

* Better naming * Reflects the schema missing parts on preview * Tested

didierofrivia self-assigned this Mar 18, 2019

didierofrivia force-pushed the feature/show-full-schema-preview branch from 9f037b5 to b6ca6ed Compare March 18, 2019 16:13

hallelujah reviewed Mar 19, 2019

View reviewed changes

didierofrivia force-pushed the feature/show-full-schema-preview branch from b6ca6ed to c74c873 Compare March 19, 2019 14:36

didierofrivia changed the title ~~[WIP] [Custom Policies] Show full schema info on form preview~~ [Custom Policies] Show full schema info on form preview Mar 19, 2019

didierofrivia requested review from hallelujah, thomasmaas, damianpm and josemigallas March 19, 2019 15:02

thomasmaas reviewed Mar 19, 2019

View reviewed changes

damianpm reviewed Mar 20, 2019

View reviewed changes

didierofrivia force-pushed the feature/show-full-schema-preview branch from 4221516 to 8ec8dec Compare March 20, 2019 13:46

damianpm reviewed Mar 20, 2019

View reviewed changes

didierofrivia force-pushed the feature/show-full-schema-preview branch from 8ec8dec to 3bda674 Compare March 20, 2019 14:01

Martouta requested review from thomasmaas and damianpm and removed request for thomasmaas March 20, 2019 14:05

thomasmaas previously approved these changes Mar 20, 2019

View reviewed changes

damianpm previously approved these changes Mar 20, 2019

View reviewed changes

didierofrivia added 2 commits March 20, 2019 15:58

[custom policy] Refactor: SchemaEditor

4a73b96

* Resides on a separate component * Does specific schema validation * Better error communication

[custom policy] Refactor: CustomPolicyEditor

b3451ea

* Better naming * Reflects the schema missing parts on preview * Tested

didierofrivia dismissed stale reviews from damianpm and thomasmaas via b3451ea March 20, 2019 14:58

didierofrivia force-pushed the feature/show-full-schema-preview branch from 3bda674 to b3451ea Compare March 20, 2019 14:58

didierofrivia requested review from thomasmaas and damianpm and removed request for thomasmaas March 20, 2019 14:59

didierofrivia requested a review from thomasmaas March 20, 2019 14:59

damianpm approved these changes Mar 20, 2019

View reviewed changes

didierofrivia merged commit 2121dee into master Mar 20, 2019

didierofrivia deleted the feature/show-full-schema-preview branch March 20, 2019 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Custom Policies] Show full schema info on form preview #712

[Custom Policies] Show full schema info on form preview #712

didierofrivia commented Mar 18, 2019 •

edited

hallelujah Mar 19, 2019

didierofrivia Mar 19, 2019

hallelujah Mar 19, 2019

didierofrivia Mar 19, 2019

codecov bot commented Mar 19, 2019 •

edited

thomasmaas Mar 19, 2019

hallelujah Mar 20, 2019

thomasmaas Mar 19, 2019

damianpm Mar 20, 2019

didierofrivia Mar 20, 2019

damianpm Mar 20, 2019

	'summary': 'A brief description of what it does.',
	'summary': 'A one-line summary of what this policy does.',

[Custom Policies] Show full schema info on form preview #712

[Custom Policies] Show full schema info on form preview #712

Conversation

didierofrivia commented Mar 18, 2019 • edited

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

codecov bot commented Mar 19, 2019 • edited

Codecov Report

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

didierofrivia commented Mar 18, 2019 •

edited

codecov bot commented Mar 19, 2019 •

edited