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

handle situation when prev and next orders are empty; fix#225 #226

Merged
merged 2 commits into from Oct 15, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -44,13 +44,19 @@ const getPrevOrderFromNextOrder = (lastNumber, order) => {
* Returns current bookmark order based on previous and next bookmark order.
* @param {String} prevOrder
* @param {String} nextOrder

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Oct 12, 2018

Member

please document parentOrder in these comments (using jsdoc)

This comment has been minimized.

Copy link
@AlexeyBarabash

AlexeyBarabash Oct 13, 2018

Author Contributor

thanks, this was fixed

* @param {String} parentOrder
* @returns {String}
*/
module.exports.getBookmarkOrder = (prevOrder, nextOrder) => {
module.exports.getBookmarkOrder = (prevOrder, nextOrder, parentOrder) => {
let prevOrderSplit = prevOrder.split('.')
let nextOrderSplit = nextOrder.split('.')

if (prevOrderSplit.length === 1 && nextOrderSplit.length === 1) {
throw new Error(`Invalid previous and next orders: ${prevOrderSplit} and ${nextOrderSplit}`)
if (!parentOrder) {

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2018

Contributor

we should handle the case where all 3 are blank (first node added) - we really have no use or need for the base order in the client and I think we should encapsulate all the logic here so the client doesn't have to know or care about how the order works, so I think we should store it in the lib and use it for this case

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2018

Contributor

we have a workaround for now in brave-core so that could be done in a follow up

throw new Error(`Invalid previous, next and parent orders: ${prevOrderSplit}, ${nextOrderSplit} and ${parentOrder}`)
} else {
return parentOrder + '.1'
}
}
let order = ''
if (nextOrderSplit.length === 1) {
@@ -117,13 +117,14 @@ const messages = {
/**
* browser -> webview
* browser sends this to get a bookmark order based on prev and next bookmark orders.
* parentOrder should be set if both prevOrder and nextOrder
*/
GET_BOOKMARK_ORDER: _, /* @param {string} prevOrder, @param {string} nextOrder */
GET_BOOKMARK_ORDER: _, /* @param {string} prevOrder, @param {string} nextOrder, @param {string} parentOrder */
/**
* webview -> browser
* webview sends this to set a bookmark order based on prev and next bookmark orders..
*/
SAVE_BOOKMARK_ORDER: _ /* @param {string} order, @param {string} prevOrder, @param {string} nextOrder */
SAVE_BOOKMARK_ORDER: _ /* @param {string} order, @param {string} prevOrder, @param {string} nextOrder, @param {string} parentOrder */
}

module.exports = mapValuesByKeys(messages)
@@ -183,10 +183,10 @@ const startSync = (requester) => {
logSync(`Getting bookmarks base order`)
ipc.send(messages.SAVE_BOOKMARKS_BASE_ORDER, bookmarkUtil.getBaseBookmarksOrder(deviceId, platform))
})
ipc.on(messages.GET_BOOKMARK_ORDER, (e, prevOrder, nextOrder) => {
logSync(`Getting current bookmark order based on prev and next orders`)
ipc.on(messages.GET_BOOKMARK_ORDER, (e, prevOrder, nextOrder, parentOrder) => {
logSync(`Getting current bookmark order based on prev, next and parent orders`)

ipc.send(messages.SAVE_BOOKMARK_ORDER, bookmarkUtil.getBookmarkOrder(prevOrder, nextOrder), prevOrder, nextOrder)
ipc.send(messages.SAVE_BOOKMARK_ORDER, bookmarkUtil.getBookmarkOrder(prevOrder, nextOrder, parentOrder), prevOrder, nextOrder, parentOrder)
})
ipc.send(messages.SYNC_READY)
logSync('success')
@@ -2,7 +2,7 @@ const test = require('tape')
const bookmarkUtil = require('../../client/bookmarkUtil')

test('test bookmarkUtil', (t) => {
t.plan(18)
t.plan(19)
t.equals(bookmarkUtil.getBaseBookmarksOrder('0', 'ios'), '2.0.')
t.equals(bookmarkUtil.getBaseBookmarksOrder('0', 'android'), '2.0.')
t.equals(bookmarkUtil.getBaseBookmarksOrder('0', 'laptop'), '1.0.')
@@ -18,7 +18,8 @@ test('test bookmarkUtil', (t) => {
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10', '2.0.8.15.1'), '2.0.8.10.1')
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10', '2.0.8.11.1'), '2.0.8.10.1')
t.equals(bookmarkUtil.getBookmarkOrder('2.0.8.10.0.1', '2.0.8.15.1'), '2.0.8.10.0.2')
t.throws(() => bookmarkUtil.getBookmarkOrder('', ''), /Invalid previous and next/, 'requires at least one argument')
t.equals(bookmarkUtil.getBookmarkOrder('', '', '2.0.9'), '2.0.9.1')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '', ''), /Invalid previous, next and parent/, 'requires at least one argument')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '2.0.0'), /Invalid input order/)
t.throws(() => bookmarkUtil.getBookmarkOrder('2.0.0', ''), /Invalid input order/)
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.