Skip to content

Conversation

@mfikria
Copy link
Contributor

@mfikria mfikria commented Apr 28, 2019

No description provided.

@maxceem
Copy link
Contributor

maxceem commented Apr 29, 2019

@vikasrohit

In this task out intention to make a restriction when we create a ProductTemplate we should either define template (old way) or form (new way).

If we use Joi to validate input it works like that: in the input, we should include only one of these properties. The way when we define on of them as null looks like doesn't work with Joi and would require a custom logic. See example where we difine template and set form as null. Joi would invalidate it with error message: validation error: \"value\" contains a conflict between exclusive peers [form, template].

{
  "param": {
    "name": "name 1",
    "productKey": "productKey 1",
    "category": "generic",
    "subCategory": "generic",
    "icon": "http://example.com/icon1.ico",
    "brief": "brief 1",
    "details": "details 1",
    "aliases": ["product key 1", "product_key_1"],
    "isAddOn": true,
    "template": {
      "template1": {
        "name": "template 1",
        "details": {
          "anyDetails": "any details 1"
        },
        "others": ["others 11", "others 12"]
      },
      "template2": {
        "name": "template 2",
        "details": {
          "anyDetails": "any details 2"
        },
        "others": ["others 21", "others 22"]
      }
    },
    "form": null
  }
}

It feels that such validation is not that comfortable for us. As when we get a response form the server we can get a response like this with form: null but we cannot send such response to the server.

What do you think? Should we keep like this when cannot send a null value, but keep validation simple with Joi. Or make a custom logic for validation, but let us submit null values, which looks like easier to work with.

What is the best way from your perspective?

@maxceem
Copy link
Contributor

maxceem commented Apr 29, 2019

@mfikria do you think is it possible to let us submit null value for form or template using and validate it using Joi?

@mfikria
Copy link
Contributor Author

mfikria commented Apr 29, 2019

@maxceem so in create endpoint we restrict the values should be non-null and in update endpoint the values could be null?

@maxceem
Copy link
Contributor

maxceem commented Apr 29, 2019

@maxceem so in create endpoint we restrict the values should be non-null and in update endpoint the values could be null?

@mfikria Not exactly.

The idea is to treat null like no value. Example:

At the moment we can create/edit produtTemplate by sending a body request like this:

{
  "param": {
    "name": "name 1",
    ...other params...
    "template": {
      
    }
  }
}
  • this request is correct as we provide template but didn't provide form

But if we send requests where we provide template with JSON and form as null:

{
  "param": {
    "name": "name 1",
    ...other params...
    "template": {
      
    }
  },
  "form": null,
}
  • we will get an error like validation error: \"value\" contains a conflict between exclusive peers [form, template], as we cannot provide both together template and form. But actually we provided form as null.

So the questions is can we treat null as no value. So we can send request where template is JSON and form is null.

So we can rephrase the initial task to:

  • form or template should be sent as not null (some JSON), but both together cannot be not null. So one of them should be null or not defined at all.

Is it possible to achieve it using Joi without writing a custom logic for this?

@mfikria
Copy link
Contributor Author

mfikria commented Apr 29, 2019

@maxceem yes it could be achieved, i updated the PR. Btw the error message can be override by updating JOI version so it has this feature https://github.com/hapijs/joi/blob/master/API.md#anyerrorerr-options . sample: https://stackoverflow.com/a/54657686

@maxceem
Copy link
Contributor

maxceem commented Apr 29, 2019

@mfikria great, thank you! I'll check it out tomorrow. The message error I think is ok for now, but thanks for mentioning it.

@maxceem maxceem changed the base branch from dev to feature/project-product-templates-final-fixes April 30, 2019 02:01
@maxceem maxceem self-requested a review April 30, 2019 02:26
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works good.

Thank you @mfikria for improving unit tests also.

@maxceem maxceem merged commit 866c8f5 into topcoder-platform:feature/project-product-templates-final-fixes Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants