informatics-isi-edu / ermrestjs

ERMrest client library in JavaScript
Apache License 2.0
4 stars 3 forks source link

Review the logic to hide "No value" option in facets #925

Closed RFSH closed 7 months ago

RFSH commented 2 years ago

When we introduced the No Value (null) option in facets, we also used some heuristics that will make sure the "No value" is only offered in the cases that makes sense.

In this issue, our goal is to review the logic and see whether we should make any changes to it. For reference, #728 is the original issue where we discussed the addition of this feature and in #888 we made some modifications to it.

In faceting a No value filter could mean any of the following:

Since we're not going to show two different options for these two, we have to make sure to offer No value option when only one of these two meanings would make sense. Based on this, we can categorize facets into these groups:

  1. (G1) Facets without any hops where column is nullable.

  2. (G2) Facets without any hops where column is not-null.

  3. (G3) All outbound foreign key facets that all the columns involved are not-null.

  4. (G4) Facets with hops where column is nullable. In this case No value could mean both scalar value null and path existence check. Therefore we cannot offer No value for them.

  5. (G5) For other cases No Value can only mean the path existence check and therefore we can offer that option to the users. Since this is path existence check, we have to use the outer join and therefore multiple of them cannot co-exist.

Based on this, the following is the logic to offer or not offer the "No value" option (first applicable rule):

I should mention that apart from checking the model, we used to also check user ACLs and whether use is looking at snapshot or not, because technically ERMrest could return null value for a not-null column. But we realize that even though ERMrest technically can return null in these two cases, this null value is not the general intended use case for this filter. And therefore we should remove the check for these two cases.

To provide more information:

RFSH commented 7 months ago

I'm going to close this issue as it's old, and we've made changes to this logic ever since.