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

[Workbase] - Implicit relationship bug #2956

Merged
merged 4 commits into from Aug 16, 2018

Conversation

Irtazaraza
Copy link
Contributor

Why is this PR needed?

Implicit relationships were being loaded to the graph

What does the PR do?

Filter outs all implicit types

Does it break backwards compatibility?

No

List of future improvements not on this PR

No

@Irtazaraza Irtazaraza changed the title Implicit relationship bug [Workbase] - Implicit relationship bug Aug 15, 2018
@@ -106,7 +109,7 @@ const methods = {
if (!concepts.length) {
this.$notifyInfo('No results were found for your query!', 'bottom-right');
}
return { nodes: await ManagementUtils.prepareNodes(concepts), edges: [] };
return ManagementUtils.filterImplicitTypes(concepts);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why we don't filter out implicit nodes right after const concepts = await this.exectueQuery(query);?

return Promise.all(concepts.map(async (concept) => {
if (concept.isThing()) {
return (!await (await concept.type()).isImplicit()) ? concept : null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also filter out implicit schema concepts? just to be more consistent,
so maybe just add:

if (concept.isSchemConcept()) {
      return !await concept.isImplicit() ? concept : null;
    }

or maybe there is a reason why we are not doing it - I am just wondering

await prepareRelationship(thing);
break;
default:
// do nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

throw "Unrecognised baseType of thing ["+thing.baseType+"]" we should never get anything that is not entity/rel/attribute - if we ever do let's throw exception so that these things don't go unnoticed

expect(concepts).toHaveLength(1);
concepts = await DataManagementUtils.filterImplicitTypes(concepts);
expect(concepts).toHaveLength(0);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

YAAAYY testssss 👍 💯

@marco-scoppetta marco-scoppetta merged commit 54fcf79 into vaticle:master Aug 16, 2018
@Irtazaraza Irtazaraza deleted the implicit-relationship-bug branch October 4, 2018 15:51
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.

None yet

2 participants