provide better error message when attempt is made to add bookmark folder to root folder

RESOLVED FIXED in Firefox 68

Status

enhancement
P5
normal
RESOLVED FIXED
6 months ago
Last month

People

(Reporter: dietrich, Assigned: myeongjun.ko, Mentored)

Tracking

({dev-doc-needed, good-first-bug})

unspecified
mozilla68
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

6 months ago
And update docs too, ty!
Could you provide some more detail here so that someone new could pick it up actionably? Thanks!
Flags: needinfo?(dietrich)
Keywords: good-first-bug
Priority: -- → P5
Reporter

Comment 2

6 months ago
Sure! John-galt asked me to file it, so he will probably let us know what is correctly actionable, but some details on my experience are below.

Pseudo-code:

let root = (await bookmarks.getTree())[0];

let myFolder = await bookmarks.create({ title: "my folder", parentId: root.id });

This throws an error because the policy is that extensions cannot add items to the root node.

Two things should probably happen:

1. The error message doesn't say anything helpful. I can't remember what it says, but was nothing related to the root (hehe) cause of the error - or at least no discernible to me. The error message should be updated to inform the developer what has happened.

2. I found out about the policy by asking on IRC, even after reading through to docs. There should probably be documentation for it.
Flags: needinfo?(dietrich)
If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started. 

Mentor: Rob Wu
Mentor: rob

Comment 4

4 months ago

Hello, i would like to try this out as my first contribution

Flags: needinfo?(rob)

Comment 5

4 months ago

Hi Pollet, comment 2 has described a test case to manually check the behavior of the bookmarks API.
This is extension code, so if you aren't familiar with the area yet, check out https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions

To get started with setting up a development environment and submitting a patch, take a look at the tutorial at
https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

The relevant code is in browser/components/extensions/parent/ext-bookmarks.js , which you can view at https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-bookmarks.js

If you have any other questions, feel free to needinfo me.

Flags: needinfo?(rob)

Comment 6

4 months ago

okay thank you

Assignee

Comment 7

2 months ago

Is this bug don't solve yet?
When create function call, create function check if it is root node. And Is it throw error such as throw new Error("This id is root node")?

Assignee

Comment 8

2 months ago

(In reply to Myeongjun Go from comment #7)

Is this bug don't solve yet?
When create function call, create function check if it is root node. And Is it throw error such as throw new Error("This id is root node")?

I didn't refer to mentor.
Please, Check my comment. If it is correct and pollet doesn't try anymore, I wanna try to solve it.

Flags: needinfo?(rob)

Comment 9

2 months ago

The bug is still open. The fix that you described sounds right!

This is not only an issue with the browser.bookmarks.create method, but also every other method of the bookmarks API. So to fix the bug, please create a helper function that throws ExtensionError("The root folder cannot be modified") when the given ID is the root ID, and call this helper function for each of the following methods (for the properties/variables between parentheses):

  • create (bookmark.parentId)
  • move (id and destination.parentId)
  • update (id)
  • remove (id)
  • removeTree (id)

You should also include a unit test to verify that the fix is working as expected, for example by appending test cases to https://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js

Flags: needinfo?(rob)
Assignee

Comment 10

2 months ago

Thanks for replying :)
I start to work on issue. I'm going to make helper function right over create function. Function name is going to be named checkRootId(id). Or, You could recommend proper name.

Flags: needinfo?(rob)

Comment 11

2 months ago

I would go with throwIfRootId.

Once you've submitted a patch to Phabricator I'll assign this bug to you :)

Flags: needinfo?(rob)
Assignee

Comment 13

2 months ago

(In reply to Myeongjun Go from comment #12)

I fixed it :)
If It is needed to revise after you're review patch, Tell me about problems.

Flags: needinfo?(rob)

Comment 14

2 months ago

Thanks for the patch! I'll take a look.

Assignee: nobody → myeongjun.ko
Status: NEW → ASSIGNED
Flags: needinfo?(rob)
Assignee

Comment 15

2 months ago

I fixed it again and submit patch.
Thank you very much about reviewing kindly :)

Flags: needinfo?(rob)

Comment 16

2 months ago

You're welcome, and thanks for your contribution!

The next step is getting a review from a module peer (which I'm not; I've added Luca to your review - expect a reaction tomorrow or next week).
Once he has approved your patch, you can edit this bug and add the checkin-needed keyword to request a check-in of your patch.

Before requesting a check-in, do make sure that your patch passes the relevant tests (see https://searchfox.org/mozilla-central/search?q=browser.bookmarks&case=false&regexp=false&path=extensions%2Ftest ).

Flags: needinfo?(rob)

Comment 17

2 months ago

Added dev-doc-needed keyword, as we should also update the API docs on MDN to mention this behavior explicitly.

Keywords: dev-doc-needed
Assignee

Comment 18

2 months ago

Should I update the api docs on MDN to merge my patch?

Flags: needinfo?(rob)
Assignee

Updated

2 months ago
Keywords: checkin-needed

Comment 19

2 months ago

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/185cd658ce9a
Provide better error message when attempt is made to add bookmark folder to root folder. r=robwu,rpl

Keywords: checkin-needed

Comment 20

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment 21

2 months ago

(In reply to Myeongjun Go from comment #18)

Should I update the api docs on MDN to merge my patch?

The documentation is only updated after a patch lands.

In this case, it seems best to add a brief explanation as a new (third) paragraph of the introduction section at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/bookmarks

Flags: needinfo?(rob)

Congrats on landing the patch, Myeongjun! 🎉 Your contribution has been added to our contribution wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition and your Mozillians.org profile has been vouched. 🚀

Welcome onboard! I look forward to seeing you around the project.

Can you please provide some STR for this issue so we can check it manually? If no manual testing is needed please mark it as "qe-verify- "

Flags: needinfo?(rob)

Covered by automated tests, so qe-verify-.

Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.