Closed Bug 1512171 Opened 2 years ago Closed 1 year ago

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

Categories

(WebExtensions :: General, enhancement, P5)

enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

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

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(1 file)

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
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

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

Flags: needinfo?(rob)

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)

okay thank you

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")?

(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)

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)

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)

I would go with throwIfRootId.

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

Flags: needinfo?(rob)

(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)

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

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

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

Flags: needinfo?(rob)

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)

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

Keywords: dev-doc-needed

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

Flags: needinfo?(rob)
Keywords: checkin-needed

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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(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-

Here are the updates I made. I'm not entirely sure it needs to be formatted as a warning. Let me know if you would like to change that or make it a note instead. Also, please review my changes and let me know if I missed anything!

  • Added the following warning to Bookmarks API:

    Extensions cannot create, modify, or delete bookmarks in the root node of the bookmarks tree. Doing so causes an error with the message: "The bookmark root cannot be modified"

  • Added the following warning to the bookmarks.create method:

    If your extension tries to create a new bookmark in the bookmark tree's root node, it raises an error: "The bookmark root cannot be modified" and the bookmark won't be created.

  • Added the following warning to bookmarks.move method:

    If your extension attempts to move a bookmark into the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the move won't be completed.

  • Added the following to warning bookmarks.remove:

    If your extension attempts to remove a bookmark from the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be removed.

  • Added the following message to bookmarks.removeTree:

    If your extension attempts to remove a bookmark tree from the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be removed.

  • Added the following message to bookmarks.update:

    If your extension attempts to update a bookmark in the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be updated.

Flags: needinfo?(rob)

Looks good, thanks!

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