Skip to content

Improvements for product/project template refactoring #297

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

Merged
merged 2 commits into from
Apr 30, 2019

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