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 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

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

  • Loading branch information
AlexeyBarabash committed Oct 12, 2018
commit 52b53d9e6fc32561e143e0959d00045cb63c117c
@@ -46,11 +46,15 @@ const getPrevOrderFromNextOrder = (lastNumber, order) => {
* @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

* @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 and next 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.throws(() => bookmarkUtil.getBookmarkOrder('', '', ''), /Invalid previous, next and parent/, 'requires at least one argument')
t.throws(() => bookmarkUtil.getBookmarkOrder('', '', '2.0.9'), '2.0.9.1')

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Oct 12, 2018

Member

this should not throw

This comment has been minimized.

Copy link
@AlexeyBarabash

AlexeyBarabash Oct 13, 2018

Author Contributor

thanks, this was fixed also

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.