tnc-ca-geo / animl-frontend

A frontend web app for viewing & labeling camera trap data by The Nature Conservancy.
https://animl.camera
Other
16 stars 0 forks source link

Give users option to explicitly include/exclude future cameras/deployments/labels into filter selections & views #38

Open nathanielrindlaub opened 3 years ago

nathanielrindlaub commented 3 years ago

Currently when creating a view that includes all currently available cameras & deployments, the filters saved for that view for cameras & deployments will be "null", which means that all new cameras/deps that get added after that will be included in that view.

Similarly, if you have ANY deployments or labels toggled off (for example if you created a view that excluded empty images), any new labels that were added after the view was created would NOT be included in that view.

nathanielrindlaub commented 3 years ago

Will have to update all existing views in prod manually or re-save them once this is implemented.

nathanielrindlaub commented 2 years ago

I implemented this, but the code started to feel gross, so I thought about it a bit more I've realized that the crux of the issue is that a null value for filters who's values are typically arrays of Ids (e.g. cameras, deployments, labels) is fundamentally different from that type of filter equaling an array of every currently available Id. A null value on an array-type filter like deployments or labels is similar to a null filter for createdDate: it is future camera/deployment/label inclusive. That might actually be what users expect or intend when they select all deployments, create a view, and set an automation rule for the view. They might be thinking "please apply this rule to all current deployments but also all future deployments & cameras added to the project as well".

On the other hand, they also might expect the opposite and be surprised when a future deployment or label gets included and gets the same treatment as the others. I think it's a mistake to conflate the two, and that we should expose the choice to users.

It wouldn't be too hard to implement; a checkbox next to the bulk select checkboxes that reads "include all future [deployments]" that could become active when all are already selected would do the trick. If the "include all future" checkbox is checked, set that filter to null, otherwise treat it as an explicit array of Ids.

I'm going to de-prioritize this b/c it doesn't seem too urgent, however.

I don't think this will be too helpful for implementing this down the road, but just in case, the updates I am about to discard that I made when I was assuming that users did not want to include future labels/deployments were the following:

  1. create a src/features/views/utils.js file with the following:
    
    // if 'cameras' or 'deployments' filters === null, 
    // replace the null with an array of all currently available ids

const burnFiltersIntoView = (filtersToSave, availFilters) => { const filtCatsToBurnIntoView = ['cameras', 'deployments']; let updatedFilters = {}; for (const cat of filtCatsToBurnIntoView) { const availIds = availFilters[cat].ids; updatedFilters[cat] = filtersToSave[cat] === null ? availIds : filtersToSave[cat]; } return {...filtersToSave, ...updatedFilters}; // NOTE: important to use spread syntax here b/c some of the filters properties are non-writable, so we need to clone them into writable props };

export { burnFiltersIntoView, };


2. burn the array of ids into views when saved in the `editView` thunk:
```Javascript
export const editView = (operation, payload) => {
  return async (dispatch, getState) => {
    try {
      dispatch(editViewStart());
      switch (operation) {
        case 'create': {
          const availFilters = getState().filters.availFilters;
          payload.filters = burnFiltersIntoView(payload.filters, availFilters);  // this was the update
          const res = await call('createView', payload);
          const view = res.createView.view;
          dispatch(saveViewSuccess(view));
          dispatch(setSelectedView({ view }));
          break;
        }
        case 'update': {
          // TODO: would want to also burn filters into view here too if payload.diffs included filters
          const res = await call('updateView', payload);
          const view = res.updateView.view;
          dispatch(saveViewSuccess(view));
          dispatch(setSelectedView({ view }));
          break;
        }
       ...
  1. and finally (this took me forever to figure out), we also need to use the spread operator to copy action.payload.view.filters before dispatching setActiveFilters in setSelectedViewMiddleware due to some of the filters in the payload containing non-writable properties.
    
    export const setSelectedViewMiddleware = store => next => action => {
    if (setSelectedView.match(action)) {
    ...
    else {
      // else, set active filters to selected view filters 
      const newFilters = {...action.payload.view.filters};  // NOTE: use spread operator here
      store.dispatch(setActiveFilters(newFilters));
    }
    next(action);
    ...
    };
nathanielrindlaub commented 1 year ago

B/c we are bumping up automation rules to the project-level (#83) and de-emphasizing the role that views play (#133), I think this is now low priority.