Skip to content
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

Entity type #243

Merged
merged 12 commits into from
Jun 16, 2018
Merged

Entity type #243

merged 12 commits into from
Jun 16, 2018

Conversation

BreadAndRoses95
Copy link
Contributor

@BreadAndRoses95 BreadAndRoses95 commented Jun 7, 2018

Made regex entity not highlightabled in the user says rows. We may still need to color them automatically in the user says when someone add regex entity in the slot part. I've seen @dcalvom speaking of making an api endpoint to highlight entities in user says part, this could be a solution.

Plus, I think I should change entity creation page when regex is selected to only allow users to add a list of regexs (that will be saved inside the examples part of the entity object)? Maybe we could keep the synonyms to allow value and original regex as slots found?

@BreadAndRoses95
Copy link
Contributor Author

BreadAndRoses95 commented Jun 8, 2018

Now it's possible to add synonyms of regex entities that will follow this behavior : if a synonym of a regex entity is matched, the corresponding slot.regexEntity.value is filled with the value of the entity (not the synonym) and the original with the original value which can be a synonym.
Plus regex entity are automatically highlighted in the user says part

@wrathagom
Copy link
Contributor

Working pretty well for me. I am having some weird alignment and console errors, but I'm going to rebuild my Docker images tomorrow to see if the problem isn't on my side.

@dcalvom would we want tests for the API side of things? I'm assuming yes.

@BreadAndRoses95
Copy link
Contributor Author

BreadAndRoses95 commented Jun 12, 2018

Well nothing on my side except the
Failed to load resource: the server responded with a status of 404 (Not Found) :7500/intent/1/postFormat Failed to load resource: the server responded with a status of 404 (Not Found)

But I think that was here before. Maybe we should highlight multiple different occurrences when isList (and only the last one as this is the one added in last in the entities list and will override the other) is selected and loop over multiple different occurences on the backend side to allow matching several times different values of the same entities, let me know if this suits you and I could implement this.

For the synonym part right now we extract the resolved regex and put it into both original and value (which is not useful) but I think we should put the regex pattern into value and the resolved regex (which can be a term or a regex associated with the synonym) into original (Done)

Copy link
Collaborator

@dcalvom dcalvom left a comment

Choose a reason for hiding this comment

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

Please fix the import endpoint to accept the new type attribute in entities and run the unit tests npm run test in the api folder

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "child \"entities\" fails because [\"entities\" at position 0 fails because [\"type\" is not allowed]]",
  "validation": {
    "source": "payload",
    "keys": [
      "entities.0.type"
    ]
  }
}

@@ -188,6 +195,12 @@ export class EntityPage extends React.PureComponent { // eslint-disable-line rea
displayColorPicker,
};

let typeSelect = [];
const defaultOptionType = { value: 'learned', text: 'learned', disabled: 'disabled' };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change the first letter of the value to a caps "Learned" and "Regex"

Also given that these values affect the table for examples, it would be nice to edit the tooltips for help to let the user understand the meaning of the values he is going to enter

@dcalvom
Copy link
Collaborator

dcalvom commented Jun 16, 2018

The functionality works correctly! Nice job @BreadAndRoses95 there is just fix the small observations I did and we are good to merge.

@BreadAndRoses95
Copy link
Contributor Author

BreadAndRoses95 commented Jun 16, 2018

All right all tests passed and I included what I described before, have a look on the tooltip message, I do not think this is very clear but had no imagination to fill it properly ...

@dcalvom dcalvom merged commit 7965eea into samtecspg:master Jun 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants