axa-group / nlp.js-app

MIT License

132 stars 58 forks source link

Enable intent edit name #54

Closed maximebeaudoin closed 4 years ago

maximebeaudoin commented 5 years ago

Pull Request Template

PR Checklist

[x] I have run npm test locally and all tests are passing.
[ ] I have added/updated tests for any new behavior.
[ ] If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

To be able to use the app properly we really need to be able to change intent name over time. This behavior is common in other NLU platform like Dialogflow and Watson.

ericzon commented 4 years ago

First of all, thanks for contributing. This feature will also require at least two validations:

to prevent saving a modified intent without name
to prevent saving a modified intent with an already used name

maximebeaudoin commented 4 years ago

@ericzon I'll take a look. Do you know the best place to put this kind of validation ? By the way, when we create a intent, we can actually have duplicate intent name, even in the same domain so i'm not sure if it's a normal behavior or not.

ericzon commented 4 years ago

@maximebeaudoin as this is a kind of validation that requires db communication, I suggest you to add this checking in server/feats/intent/intent.controller.js / updateById method. By the way, related with your last comment... no, it shouldn't allow to create an intent with the same name. If you want, add the same kind of validation in .... / add method

maximebeaudoin commented 4 years ago

@ericzon I have added some kind of validation before saving. However, i'm not sure the best way to handle the 400 code into the UI. For now, it's return a Unknow api error to the user and it's not the best for them.

ericzon commented 4 years ago

Great @maximebeaudoin . I've checked the code and it looks good to me. Solve the conflicts and we'll merge it. With the current version, it'll show an error message of 'Bad request', is not perfect but I prefer to add your changes and we'll focus in these details after.

maximebeaudoin commented 4 years ago

@ericzon Conflicts resolved

maximebeaudoin commented 4 years ago

@ericzon Merging is coming soon ?