Fix for lack of id schema being generated in schema dependency demo #819

redixhumayun · 2018-01-22T07:30:47Z

Reasons for making this change

A new idSchema needs to be generated in the ObjectField.js file, because the retrieveSchema function does not resolve nested dependencies further than one level deep in the JSON object.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
- [X ] I've run npm run cs-format on my branch to conform my code to prettier coding style
I'm adding a new feature
- I've updated the playground with an example use of the feature

redixhumayun · 2018-01-29T05:51:17Z

@edi9999 @jonathonlui Hi, any updates on this?

edi9999 · 2018-01-22T08:10:26Z

src/components/fields/ObjectField.js

+      definitions,
+      formData,
+      idSchema["$id"]
+    );


Can you add a test that shows that it failed before ?

@redixhumayun you can use or adapt the test of ObjetField from my branch: master...jonathonlui:fix-nested-object-dynamic-conditionals

When ObjectField.js does not have these changes applied then using npm test the test correctly fails . However, there is an issue with the test when running npm run tdd: the first test run will correctly fail but when the test is rerun because of a changed file, the test will incorrectly pass. I'm guessing it's related to resetting the console.error stub but I was not able to resolve the issue.

@edi9999 - Hi added a failing and passing test to ObjectField_test.js. Take a look when you get a chance.

…ma and then toIdSchema

jamesdebolt · 2018-04-04T19:56:59Z

Any updates on this PR? I am needing required fields within a schema dependency a level down, and this seems to be the fix. @edi9999

edi9999 · 2018-04-05T05:37:09Z

The CI is failing : https://travis-ci.org/mozilla-services/react-jsonschema-form/jobs/336896307

jamesdebolt · 2018-04-05T14:01:40Z

@redixhumayun Can you check out why the test is failing? Thanks everyone for their work here!

redixhumayun · 2018-04-06T04:10:15Z

@jamesdebolt - The CI is failing because I was asked to add a test that showed the failure I was trying to fix by @edi9999. You can see that in the thread below the commit
Is this ready to be accepted and merged?

edi9999 · 2018-04-06T05:27:06Z

package.json

@@ -16,7 +16,8 @@
    "publish-to-npm": "npm run build:readme && npm run dist && npm publish",
    "start": "node devServer.js",
    "tdd": "cross-env NODE_ENV=test mocha --compilers js:babel-register --watch --require ./test/setup-jsdom.js test/**/*_test.js",
-    "test": "cross-env NODE_ENV=test mocha --compilers js:babel-register --require ./test/setup-jsdom.js test/**/*_test.js"
+    "test": "cross-env NODE_ENV=test mocha --compilers js:babel-register --require ./test/setup-jsdom.js test/**/*_test.js",
+    "test_ObjectField": "cross-env NODE_ENV=test mocha --compilers js:babel-register --require ./test/setup-jsdom.js test/**/ObjectField_test.js"


I think you can remove this from package.json

edi9999 · 2018-04-06T05:27:46Z

test/ObjectField_test.js

@@ -381,4 +382,76 @@ describe("ObjectField", () => {
      expect(node.querySelector("#title-")).to.be.null;
    });
  });
+
+  describe.only("schema generation for dependencies", () => {


Please remove this describe.only

To replace it by a describe

edi9999 · 2018-04-06T05:30:49Z

test/ObjectField_test.js

+      const definitions = {};
+      const formData = { a: { c: 1234 } };
+      let result = retrieveSchema(schema, definitions, formData);
+      console.log(toIdSchema(result, null, definitions, formData));


Please remove the console.log in all your code

edi9999 · 2018-04-06T05:31:20Z

test/ObjectField_test.js

+            properties: {
+              b: { type: "string" },
+              c: { type: "number" },
+              d: { type: "number" },


Please update the expectation so that the test is passing with your fix.

edi9999 · 2018-04-06T05:36:16Z

@jamesdebolt - The CI is failing because I was asked to add a test that showed the failure I was trying to fix by @edi9999. You can see that in the thread below the commit
Is this ready to be accepted and merged?

@redixhumayun I mean a test that would fail without your fix, but with your fix, it should pass (regression test).
The process I usually follow to write these kind of tests is as follows :

I have a bug, which I am trying to understand, leading to bad behavior
I write a test that fails with the current version of the code (I didn't modify the source code yet). This test describes the correct behavior.
I fix the given bug, and verify that the test that I added now succeeds.

Alligator · 2018-08-07T10:40:21Z

@redixhumayun Hey, are you going to work on the PR any further? I need this fix and it doesn't look like much more needs to be done to get this over the line.

I don't mind doing them myself in a new PR if needs be.

edi9999 · 2018-08-07T11:51:52Z

@Alligator I think you can do the fix in a new MR.

redixhumayun · 2018-08-07T17:16:21Z

@edi9999 Hi, sorry I completely forgot about this. My apologies. If @Alligator is not going to be picking this up, I don't mind completing this. I just need to remove the failing test case. Let me know.

epicfaace · 2019-01-19T02:08:02Z

@redixhumayun Do you plan to complete this?

epicfaace · 2019-08-11T04:32:01Z

Closing this -- and I believe #1320 may have already fixed the issue that this PR was initially meant to address? Is that right, @redixhumayun ?

Zaid Humayun and others added 7 commits November 28, 2017 14:23

Update for generating new idSchema based on dependencies

41ee1c6

removed extraneous npm command

9f7e8c9

Removed comment

013768a

Removed package-lock.json

b1a14ae

Merge remote-tracking branch 'upstream/master'

abef5be

Another linter fix

c2eb9e2

Fix for no id being generated in schema dependency demo

d14c608

redixhumayun mentioned this pull request Jan 22, 2018

id and for are missing from input and label when using dependencies #803

Closed

2 tasks

edi9999 reviewed Jan 29, 2018

View reviewed changes

Passing and failing test for JSON schema being passed to retrieveSche…

a0fe453

…ma and then toIdSchema

edi9999 reviewed Apr 6, 2018

View reviewed changes

epicfaace added the awaiting changes label Jan 19, 2019

epicfaace closed this Aug 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for lack of id schema being generated in schema dependency demo #819

Fix for lack of id schema being generated in schema dependency demo #819

redixhumayun commented Jan 22, 2018

redixhumayun commented Jan 29, 2018

edi9999 Jan 22, 2018

jonathonlui Jan 29, 2018

redixhumayun Feb 5, 2018

jamesdebolt commented Apr 4, 2018 •

edited

Loading

edi9999 commented Apr 5, 2018

jamesdebolt commented Apr 5, 2018

redixhumayun commented Apr 6, 2018

edi9999 Apr 6, 2018

edi9999 Apr 6, 2018

edi9999 Apr 6, 2018 •

edited

Loading

edi9999 Apr 6, 2018

edi9999 Apr 6, 2018

edi9999 commented Apr 6, 2018 •

edited

Loading

Alligator commented Aug 7, 2018

edi9999 commented Aug 7, 2018

redixhumayun commented Aug 7, 2018

epicfaace commented Jan 19, 2019

epicfaace commented Aug 11, 2019

Fix for lack of id schema being generated in schema dependency demo #819

Fix for lack of id schema being generated in schema dependency demo #819

Conversation

redixhumayun commented Jan 22, 2018

Reasons for making this change

Checklist

redixhumayun commented Jan 29, 2018

edi9999 Jan 22, 2018

Choose a reason for hiding this comment

jonathonlui Jan 29, 2018

Choose a reason for hiding this comment

redixhumayun Feb 5, 2018

Choose a reason for hiding this comment

jamesdebolt commented Apr 4, 2018 • edited Loading

edi9999 commented Apr 5, 2018

jamesdebolt commented Apr 5, 2018

redixhumayun commented Apr 6, 2018

edi9999 Apr 6, 2018

Choose a reason for hiding this comment

edi9999 Apr 6, 2018

Choose a reason for hiding this comment

edi9999 Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

edi9999 Apr 6, 2018

Choose a reason for hiding this comment

edi9999 Apr 6, 2018

Choose a reason for hiding this comment

edi9999 commented Apr 6, 2018 • edited Loading

Alligator commented Aug 7, 2018

edi9999 commented Aug 7, 2018

redixhumayun commented Aug 7, 2018

epicfaace commented Jan 19, 2019

epicfaace commented Aug 11, 2019

jamesdebolt commented Apr 4, 2018 •

edited

Loading

edi9999 Apr 6, 2018 •

edited

Loading

edi9999 commented Apr 6, 2018 •

edited

Loading