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

Feature/improve filter messaging #840

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

buckhalt
Copy link
Member

@buckhalt buckhalt commented Dec 4, 2024

Edge stage-level filters can sometimes lead to unexpected behavior when configured with edge creation or edge display on a sociogram.

For instance:

Filter: "edges of type a must exist"
Edge creation: create edge type b

In this case, edges of type b will not be shown on the sociogram.

This PR adds warning tips to the edge creation configuration, edge display configuration, and filter configuration sections when there is a potential mismatch between the filter and the edges to be created or displayed.

@buckhalt buckhalt marked this pull request as ready for review December 5, 2024 23:08
Comment on lines 36 to 89

export const getEdgeFilters = (state) => {
const getStageValue = formValueSelector('edit-stage');
const currentFilters = getStageValue(state, 'filter');

if (!currentFilters || !currentFilters.rules) {
return [];
}
const edgeFilters = currentFilters.rules.filter((rule) => rule.type === 'edge');

return edgeFilters;
};

/**
* Compare selected edges to edge filters to determine if a warning should be shown
* @param {Array} filters - edge filters
* @param {Array} edges - selected edges
* There are four main cases to consider:
* 1. Selected edge is in the filters with rule EXISTS -- no warning
* 2. Selected edge is not in the filters with rule EXISTS -- show a warning
* 3. Selected edge is in the filters with rule DOES_NOT_EXIST -- show a warning
* 4. Selected edge is not in the filters with rule DOES_NOT_EXIST -- no warning
*/

export const getEdgeFilteringWarning = (filters, edges) => {
const existFilters = filters.filter((rule) => rule.options.operator === 'EXISTS');
const doesNotExistFilters = filters.filter((rule) => rule.options.operator === 'NOT_EXISTS');

// if any edge should show a warning, return true
return edges.some((edge) => {
const isEdgeInExistFilters = existFilters.some((rule) => rule.options.type === edge);
const isEdgeInDoesNotExistFilters = doesNotExistFilters.some(
(rule) => rule.options.type === edge,
);

// case 1
if (isEdgeInExistFilters) {
return false;
}

// case 2
if (!isEdgeInExistFilters && existFilters.length > 0) {
return true;
}

// case 3
if (isEdgeInDoesNotExistFilters) {
return true;
}

// No warning in other cases
return false;
});
};
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, so as I thought from the way it is used, this is not quite right. getFilterEdgeWarning isn't a selector. It computes a value based on two other selectors. So it is just a normal function that should live elsewhere.

You can handle this a few ways, but what I would probably do is to combine the logic into an actual selector with something like this (not tested):

Suggested change
export const getEdgeFilters = (state) => {
const getStageValue = formValueSelector('edit-stage');
const currentFilters = getStageValue(state, 'filter');
if (!currentFilters || !currentFilters.rules) {
return [];
}
const edgeFilters = currentFilters.rules.filter((rule) => rule.type === 'edge');
return edgeFilters;
};
/**
* Compare selected edges to edge filters to determine if a warning should be shown
* @param {Array} filters - edge filters
* @param {Array} edges - selected edges
* There are four main cases to consider:
* 1. Selected edge is in the filters with rule EXISTS -- no warning
* 2. Selected edge is not in the filters with rule EXISTS -- show a warning
* 3. Selected edge is in the filters with rule DOES_NOT_EXIST -- show a warning
* 4. Selected edge is not in the filters with rule DOES_NOT_EXIST -- no warning
*/
export const getEdgeFilteringWarning = (filters, edges) => {
const existFilters = filters.filter((rule) => rule.options.operator === 'EXISTS');
const doesNotExistFilters = filters.filter((rule) => rule.options.operator === 'NOT_EXISTS');
// if any edge should show a warning, return true
return edges.some((edge) => {
const isEdgeInExistFilters = existFilters.some((rule) => rule.options.type === edge);
const isEdgeInDoesNotExistFilters = doesNotExistFilters.some(
(rule) => rule.options.type === edge,
);
// case 1
if (isEdgeInExistFilters) {
return false;
}
// case 2
if (!isEdgeInExistFilters && existFilters.length > 0) {
return true;
}
// case 3
if (isEdgeInDoesNotExistFilters) {
return true;
}
// No warning in other cases
return false;
});
};
export const getShouldShowEdgeFilterWarning = (state) => {
const getStageValue = formValueSelector('edit-stage');
const currentFilters = getStageValue(state, 'filter');
const edges = getFormValue(state, 'edges.create');
if (!currentFilters || !currentFilters.rules) {
return false;
}
const edgeFilters = currentFilters.rules.filter((rule) => rule.type === 'edge');
const existFilters = edgeFilters.filter((rule) => rule.options.operator === 'EXISTS');
const doesNotExistFilters = edgeFilters.filter((rule) => rule.options.operator === 'NOT_EXISTS');
return edges.some((edge) => {
const isEdgeInExistFilters = existFilters.some((rule) => rule.options.type === edge);
const isEdgeInDoesNotExistFilters = doesNotExistFilters.some(
(rule) => rule.options.type === edge,
);
if (isEdgeInExistFilters) {
return false;
}
if (!isEdgeInExistFilters && existFilters.length > 0) {
return true;
}
if (isEdgeInDoesNotExistFilters) {
return true;
}
return false;
});
};

Alternatively, you could turn getEdgeFilteringWarning into a selector by using createSelector, and having it fetch its dependencies (filters and edges) from state using those other selectors:

export const getEdgeFilteringWarning = createSelector(
getEdgeFilters,
(state) => getFormValue(state, 'edges.create'), // Could define this as its own selector, `getPromptEdgeCreate`
(filters, edges) => {
  const existFilters = filters.filter((rule) => rule.options.operator === 'EXISTS');
  const doesNotExistFilters = filters.filter((rule) => rule.options.operator === 'NOT_EXISTS');

  // if any edge should show a warning, return true
  return edges.some((edge) => {
    const isEdgeInExistFilters = existFilters.some((rule) => rule.options.type === edge);
    const isEdgeInDoesNotExistFilters = doesNotExistFilters.some(
      (rule) => rule.options.type === edge,
    );

    // case 1
    if (isEdgeInExistFilters) {
      return false;
    }

    // case 2
    if (!isEdgeInExistFilters && existFilters.length > 0) {
      return true;
    }

    // case 3
    if (isEdgeInDoesNotExistFilters) {
      return true;
    }

    // No warning in other cases
    return false;
  });
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Because each use case of this function needs to get edges and filters in slightly different ways, I'm not sure there's an easy way to make this an actual selector. I think the most straightforward implementation is to leave the function signature the same but treat it as a utility function.

Moved to a utils file and treated as a utility func instead of selector 3422a77

Copy link
Member

@jthrilly jthrilly left a comment

Choose a reason for hiding this comment

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

Some ideas here for you. Maybe you want to review the docs on redux a bit? Let me know if you want to chat about any of the concepts. This is something I had no idea about for 2+ years, so don't worry if it is confusing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants