Add proper native bookmarks deletion

[yureitzk]

This PR addresses an issue where the old bookmark deletion function was incorrectly adding empty bookmarks to user's PubSub service. This was due to an invalid item used for deletion and a type mismatch in the getPublishedItems function. This change ensures proper bookmark deletion when the server adheres to XEP-0402.

Related issue: #3815

Before submitting your request, please make sure the following conditions are met:

• Add a changelog entry for your change in CHANGES.md
• When adding a configuration variable, please make sure to
  document it in docs/source/configuration.rst
• Please add a test for your change. Tests can be run in the commandline
  with make check or you can run them in the browser by running make serve
  and then opening http://localhost:8000/tests.html.

[jcbrand]

Thanks for your pull request!

From what I can tell, the code looks correct, but I have a few suggestions regarding structure.

I think here we can have:

this.on('remove', this.removeBookmark, this);

Note: removeBookmarkStanza makes it sound like a stanza is being removed, which is not the case.

Then in removeBookmark, you can put the logic above to determine sourceBookmark.

BTW, in this codebase we name variables with snake-case, so sourceBookmark should be source_bookmark.
I know this is not how it's done in most JS codebases, but this repo started 10 years ago before JS conventions were well established and for consistency's sake we should keep to this convention.

Also, would you be willing to look into adding tests for this?
You can add the tests here: https://github.com/conversejs/converse.js/blob/master/src/headless/plugins/bookmarks/tests/bookmarks.js

There are multiple examples which should hopefully make it clear to you how to write a test.

[yureitzk]

Yes, no problem. As for the tests, would it be okay if I make 2 separate tests for both Strophe.NS.BOOKMARKS2 and Strophe.NS.BOOKMARKS?

[jcbrand]

Yes, no problem. As for the tests, would it be okay if I make 2 separate tests for both Strophe.NS.BOOKMARKS2 and Strophe.NS.BOOKMARKS?

Yes that would be great thanks!

We put the BOOKMARKS tests inside a file called deprecated.js, see here for example in the bookmark-views plugin:
https://github.com/conversejs/converse.js/blob/master/src/plugins/bookmark-views/tests/deprecated.js

So you'll have to create that file in src/headless/plugins/bookmarks/tests/ and put those tests there.

[jcbrand]

Where did you get this markup?

According to the XEP it should look like this:

    <retract node='urn:xmpp:bookmarks:1' notify='true'>
      <item id='theplay@conference.shakespeare.lit'/>
    </retract>

See: https://xmpp.org/extensions/xep-0402.html#removing-a-bookmark

[yureitzk]

I initially put it in as a placeholder but forgot to replace it with the code from the spec because it didn't throw any errors and actually worked. I couldn't find any mentions of it being officially supported, except for the fact that a similar syntax is used for account deletion (where I got it from) and removing a contact from the WaitingList, though.

The corrected version looks like this:

if (node === Strophe.NS.BOOKMARKS2) {

	const stanza = stx`
	<iq type="set" from="${api.connection.get().jid}" xmlns="jabber:client">
	  <pubsub xmlns='http://jabber.org/protocol/pubsub'>
		<retract node="${node}">
		  <item id="${bookmark.get('jid')}"/>
		</retract>
	  </pubsub>
	</iq>`;

	return await api.sendIQ(stanza);
}

If that's okay, I can change the stanza to this in my commit

[jcbrand]

Yes please!