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

Use patch for list updates #1511

Merged
merged 8 commits into from
Feb 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions inputs/autocomplete.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<script>
import _ from 'lodash';
import item from './autocomplete-item.vue';
import { getProp, removeListItem } from '../lib/lists/helpers';
Copy link
Contributor

Choose a reason for hiding this comment

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

removeLIstItem and addListItem are no longer used (and shouldn't be used) so they can be removed

import { getItemIndex, getProp } from '../lib/lists/helpers';

export default {
props: ['args', 'select', 'query', 'focusIndex', 'updateFocusIndex', 'updateMatches', 'unselect', 'disabled'],
Expand Down Expand Up @@ -95,25 +95,21 @@
});
},
removeFromList(item) {
let stringProperty;

const listName = this.args.list;

this.unselect();

return this.$store.dispatch('updateList', {
return this.$store.dispatch('patchList', {
listName: listName,
fn: (items) => {
stringProperty = getProp(items, 'text');
const stringProperty = getProp(items, 'text'),
index = getItemIndex(items, item, stringProperty);

if (stringProperty) {
return removeListItem(items, item, stringProperty);
} else {
return removeListItem(items, item);
if (index !== -1) {
return { remove: [items[index]] };
}
}
})
.then(this.fetchListItems);
}).then(list => this.listItems = _.map(list, item => _.isObject(item) ? item.text : item));
}
},
mounted() {
Expand Down
17 changes: 9 additions & 8 deletions inputs/simple-list.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,22 @@ describe('SimpleList input', () => {
test('Should decrease count when there is a removedItem', () => {
wrapper.vm.removedItem = { text: 'test2' };

expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual([
{ text: 'test1', count: 1 },
{ text: 'test2', count: 1 }
]);
// since it exists, it decreases count
expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual({
add: [{ text: 'test2', count: 1 }],
remove: [{ text: 'test2', count: 2 }]
});
});

test("Should return same array if removedItem doesn't exist", () => {
test("Should return undefined removedItem doesn't exist", () => {
wrapper.vm.removedItem = { text: 'something else' };

expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual(items);
expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual(undefined);
});

test('Should return same array if removedItem is null', () => {
test('Should return undefined if removedItem is null', () => {
wrapper.vm.removedItem = null;

expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual(items);
expect(wrapper.vm.handleRemoveItem(items)).toStrictEqual(undefined);
});
});
74 changes: 48 additions & 26 deletions inputs/simple-list.vue
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@
import simpleListItem from './simple-list-item.vue';
import simpleListInput from './simple-list-input.vue';
import attachedButton from './attached-button.vue';
import { addListItem, getItemIndex, getProp } from '../lib/lists/helpers';
import { getItemIndex, getProp } from '../lib/lists/helpers';
import { DynamicEvents } from './mixins';

const log = logger(__filename);
Expand Down Expand Up @@ -249,7 +249,7 @@
if (this.args.autocomplete && this.args.autocomplete.allowRemove) {
const listName = this.args.autocomplete.list;

return this.$store.dispatch('updateList', {
return this.$store.dispatch('patchList', {
listName: listName,
fn: this.handleRemoveItem
});
Expand All @@ -268,57 +268,79 @@
const itemIndex = getItemIndex(items, (this.removedItem || {}).text, 'text');

if (itemIndex !== -1 && items[itemIndex][countProperty]) {
// decrease count if the item already exists in the list and count is more than 0
items[itemIndex][countProperty]--;
// decrease count if the item already exists in the list and count is more than 0

const old = items[itemIndex],
updated = _.cloneDeep(old);

updated[countProperty]--;

this.removedItem = {};

return {
add: [updated],
remove: [old]
};
}
}

return items;
},
addItem(newItem) {
let countProperty,
itemIndex,
listName,
stringProperty;

this.items.push(newItem);
this.update(this.items);

// only add items to the list if the schema allows it
if (this.args.autocomplete && this.args.autocomplete.allowCreate) {
listName = this.args.autocomplete.list;
const listName = this.args.autocomplete.list;

return this.$store.dispatch('updateList', {
return this.$store.dispatch('patchList', {
listName: listName,
fn: (items) => {
// validate that the list has items with these properties
stringProperty = getProp(items, 'text');
countProperty = getProp(items, 'count');
const patch = {
add: [],
remove: []
},
// validate that the list has items with these properties
stringProperty = getProp(items, 'text'),
countProperty = getProp(items, 'count');

if (stringProperty && countProperty) {
itemIndex = getItemIndex(items, newItem.text, 'text');
const itemIndex = getItemIndex(items, newItem.text, 'text');

if (itemIndex !== -1) {
// increase count if the item already exists in the list
items[itemIndex][countProperty]++;
// remove the old, add new object with new count
const old = items[itemIndex],
updated = _.cloneDeep(old);

return items;
patch.remove.push(old);

// increase count if the item already exists in the list
updated[countProperty]++;
patch.add.push(updated);

return patch;
} else {
// add item to the list
// add item to the list
_.set(newItem, countProperty, 1);

return addListItem(items, newItem);
patch.add.push(newItem);

return patch;
}
} else if (_.isString(_.head(items))) {
// if the list is just an array of strings, just add the string
// property
return addListItem(items, newItem.text);
// if the list is just an array of strings, just add the string property

if (getItemIndex(items, newItem.text) === -1) {
patch.add.push(newItem.text);

return patch;
}
} else if (items.length === 0) {
log.warn('The list is empty, unable to determine data structure. Adding item with default data structure.', { action: 'adding item to a list' });
_.set(newItem, 'count', 1);

return addListItem(items, newItem);
patch.add.push(newItem);

return patch;
}
}
});
Expand Down
19 changes: 19 additions & 0 deletions lib/core-data/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,25 @@ export function saveList(prefix, listName, items) {
})).then(expectJSONResult);
}

/**
* convenience function to save list data
* @param {string} prefix
* @param {string} listName
* @param {array} items
* @return {Promise}
*/
export function patchList(prefix, listName, patch) {
let uri = `${prefix}${listsRoute}${listName}`;

assertUri(uri);

return send(addJsonHeader({
method: 'PATCH',
url: uri,
body: JSON.stringify(patch)
})).then(expectJSONResult);
}

export function getMeta(uri) {
assertUri(uri);

Expand Down
43 changes: 40 additions & 3 deletions lib/lists/actions.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import _ from 'lodash';
import { LIST_LOAD_PENDING, LIST_LOAD_SUCCESS, LIST_LOAD_FAIL } from './mutationTypes';
import { getList as getListFromAPI, saveList } from '../core-data/api';
import { getList as getListFromAPI, saveList, patchList as patchListAPI } from '../core-data/api';

/**
* @module lists
Expand All @@ -16,15 +16,52 @@ export function getList(store, listName) {
.catch(error => store.commit(LIST_LOAD_FAIL, { listName, error }));
}

export function updateList(store, { listName, fn }) {
/**
* if a list doesn't exist, creates the empty list
* @param {Object} store
* @param {string} listName
* @returns {Array<Object>}
*/
function getOrStartList(store, listName) {
const prefix = _.get(store, 'state.site.prefix');

store.commit(LIST_LOAD_PENDING, { listName });

return getListFromAPI(prefix, listName)
.catch(() => []) // allow adding to a new list
.catch(error => {
// if it's a 404, that means the list doesn't exist and we need to create it
if (error && error.response && error.response.status === 404) {
return saveList(prefix, listName, []);
} else {
throw error;
}
});
}

export function updateList(store, { listName, fn }) {
const prefix = _.get(store, 'state.site.prefix');

return getOrStartList(store, listName)
.then(listItems => fn(_.cloneDeep(listItems)))
.then(listItems => saveList(prefix, listName, listItems))
Copy link
Contributor

Choose a reason for hiding this comment

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

so it looks like the remaining lists using 'updateList' are new-pages and bookmarks, neither of which will ever reach a large number of items.

However if we keep updateList in the codebase, then

  1. future clay devs may introduce it to update a large list without realizing
  2. plugins may introduce bugs by dispatching an action for a large list.

so I could see value in forcing patchList to be used moving forward (i.e. remove updateList). But no strong opinions there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea i was hesitant to remove it from the codebase in case there were already existing plugins that used it. and because of this, felt like it'd be a waste of time re-writing the bookmarks/new-pages logic.

i'll definitely rework those two functions to use patchList though, if we think it'd be beneficial

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, the trouble of moving people off updateList when it is likely fine for 99% of lists - is probably not worth it.

so yeah i'm good with leaving this as is

.then(listItems => store.commit(LIST_LOAD_SUCCESS, { listName, listItems }))
.catch(error => store.commit(LIST_LOAD_FAIL, { listName, error }));
}

export function patchList(store, { listName, fn }) {
const prefix = _.get(store, 'state.site.prefix');

// get the list
return getOrStartList(store, listName)
// pass list to fn
.then(list => {
const patch = fn(list);

// fn needs to return a patch `{ remove: [], add: [] }`
return patch && (patch.add || patch.remove) ? patchListAPI(prefix, listName, patch) : list;
})
// patch api returns full list
.then(listItems => store.commit(LIST_LOAD_SUCCESS, { listName, listItems }))
// catch and log errors
.catch(error => store.commit(LIST_LOAD_FAIL, { listName, error }));
}
27 changes: 27 additions & 0 deletions lib/lists/actions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,31 @@ describe('list actions', () => {
});
});
});

describe.only('patchList', () => {
const fn = lib.patchList,
prefix = 'domain.com',
listName = 'listName';

test('allows sends a PATCH request to api with remove and add in body', () => {
const store = {
commit: jest.fn(),
state: {
site: {
prefix
}
}
},
start = [1, 2, 3],
patch = { add: [4], remove: [1] },
end = [2, 3, 4];

api.getList.mockResolvedValue(start);
api.patchList.mockResolvedValue(end);

return fn(store, { listName: listName, fn: () => patch }).then(() => {
expect(api.patchList).toHaveBeenCalledWith(prefix, listName, patch);
});
});
});
});
44 changes: 0 additions & 44 deletions lib/lists/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,6 @@ import logger from '../utils/log';

const log = logger(__filename);

/**
* @param {array} items
* @param {string | object} testItem
* @param {string} testProperty (optional)
* @returns {array}
*/
export function addListItem(items, testItem) {
const index = getItemIndex(items, testItem);

if (index !== -1) {
log.info('This item already exists in this list.', { action: 'modifyList' });

return items;
} else {
items.push(testItem);

return items;
}
}

/**
* check that item in a list have a property that is a string or number 0 and then return that property
* @param {array} items
Expand Down Expand Up @@ -86,30 +66,6 @@ export function getItemIndex(items, testItem, testProperty) {
return index;
}

/**
* @param {array} items
* @param {string | object} deletedItem
* @param {string} testProperty (optional)
* @returns {string}
*/
export function removeListItem(items, deletedItem, testProperty) {
let index;

if (testProperty) {
index = getItemIndex(items, deletedItem, testProperty);
} else {
index = getItemIndex(items, deletedItem);
}

if (index !== -1) {
items.splice(index, 1);
} else {
log.error(`Cannot remove ${deletedItem} because it is not in the list.`, { action: 'modifyList' });
}

return items;
}

/**
* when fetching or updating pages, make sure they're sorted
* note: this will also place them into a General category if they don't currently have categories
Expand Down
Loading