WIP: Block users #3164
WIP: Block users #3164
Conversation
udanieli
force-pushed
the
block-users
branch
3 times, most recently
from
af9835a
to
5b67845
Compare
|
I am stepping forward to make the blocklist feature usable. As you can see, it's a work in progress. Sorry for not adding tests (yet), I am more of a backend guy, not a JS expert. I have just seen how you did elsewhere in the codebase and I am trying to do the same. Moreover, I learnt Javascript in the Stone Age so I must acquaint myself with all the evolutions of the language. Any advice is welcome. |
|
Hi @udanieli, thanks a lot for your contribution. I had a quick look and left a review comment. Can you please also squash all your commits into a single commit? This will make it much easier to incorporate your changes. In the branch you were referring to, conversejs:jcbrand/block-users, I was attempting to clean up and improve the code contributed by someone else. To be honest, there are still quite a few things to improve there (beyond what you've already done in this MR), but looks like I ran out of steam and left the branch in an incomplete state. Once you've addressed the review comment and squashed your commits, I'll run this PR's branch locally and take a close look (and probably make more follow-up changes). |
* get/set blocklist with common code * add promises for blocklistFetched and blocklistUpdated events * disallow sending messages to blocked contacts
|
Ok squashed. I have also added a
Should I add that commit to this PR? Maybe I am missing translations too. Should I update the |
Having a permanent list of blocked users in the control box does not sound like good UI/UX to me. Perhaps there's a better place to put them. For example in a new modal that can be opened from the control box. Similar to the modal that shows MUC bookmarks.
I think putting it in a separate PR would keep things cleaner and more manageable.
You can run It would be best to do it after the merge. |
|
Hi @udanieli, you've done great work so far, thank you! How are you feeling so far? Are you enjoying working on Converse and motivated to continue? Or are you becoming tired? I have more feedback on this PR, but I want to be respectful of your time. |
Thank you for sharing Converse with the community.
Well, I have never written so much Javascript in my life, but these are the times we live in. Working with something I have not fully mastered is sometimes frustrating, but all in all I am happy with the result. I am still working on Converse, at the moment I am extending the
All your reviews are welcome. |
There was a problem hiding this comment.
Hi @udanieli
As promised I left more review comments.
Tests aren't running for this PR, I think because you're not basing it on master but a different branch instead.
Have you been running tests locally to see whether they're still green?
| return _converse.blocked.get('set'); | ||
| }, | ||
| }; | ||
| blocking: { |
There was a problem hiding this comment.
This already looks much better thanks. However, I think blocklist would be a better namespace than blocking.
The reason being that it allows us to write the API in a more readable way.
I would make the following renamings:
api.blocking.block->api.blocklist.addapi.blocking.unblock->api.blocklist.removeapi.blocking.blocklist->api.blocklist.get
The supported and refresh methods can stay as they are.
I realize you may have called the namespace blocking because that's the name of the plugin's folder. I don't think we generally follow a convention of keeping the plugin folder and api namespace names the same (although an argument could be made we should), at least not explicitly. In any case, if such conformity is desired, I'd rather rename the plugin folder to blocklist as well. I don't think it's necessary though.
| * | ||
| * @param { Array } [jid_list] - The list of JIDs to block | ||
| */ | ||
| block ( jid_list ) { |
There was a problem hiding this comment.
Would be nice if this first parameter can either be a string (single JID) or an array of JIDs.
Similarly to how it's done here:
Same for the function below.
| 'id': u.getUniqueId(action) | ||
| }).cnode(element); | ||
|
|
||
| const result = await api.sendIQ(iq).catch(e => { log.fatal(e); return false }); |
There was a problem hiding this comment.
I think it might be better to here handle the result stanza by calling handleBlockingStanza(result) instead of doing it via addHandler on line 46 (although we'd still need addHandler for set IQ stanzas).
We can then use await to wait for the result stanza to be processed and then in the API methods we do return sendBlockingStanza so that the promise is returned to the caller.
The advantage of doing it like this is that the caller of the API method can decide to await for the operation to have succeeded and thereby be sure that the blocklist has been updated accordingly.
| jid_list.forEach((jid) => { | ||
| const item = Strophe.xmlElement('item', { 'jid': jid }); | ||
| element.append(item); | ||
| }); |
There was a problem hiding this comment.
{ 'jid' : jid } can be shortened to { jid } and we can make this a one-liner.
jid_list.forEach((jid) => element.append(Strophe.xmlElement('item', { jid }));
| @@ -39,7 +39,8 @@ const ChatBox = ModelWithContact.extend({ | |||
| 'time_opened': this.get('time_opened') || (new Date()).getTime(), | |||
| 'time_sent': (new Date(0)).toISOString(), | |||
| 'type': _converse.PRIVATE_CHAT_TYPE, | |||
| 'url': '' | |||
| 'url': '', | |||
| 'contact_blocked': undefined | |||
There was a problem hiding this comment.
I think this is unnecessary, since you'd automatically get undefined when calling model.get('contact_blocked') if it wasn't set to something else.
And yes, I'm aware there are other undefined defaults in this object. Some of this code is so old, when I wrote it I didn't know what I know now ;-)
| const blocking_supported = await api.blocking?.supported(); | ||
| if (blocking_supported) { | ||
| await api.waitUntil("blockListFetched"); | ||
| this.checkIfContactBlocked(api.blocking.blocklist()); |
There was a problem hiding this comment.
await api.waitUntil("blockListFetched") should be called inside api.blocking.blocklist().
We don't want consumers of our API to need to know that they have to first wait for the blockListFetched promise. That's an implementation detail that we need to handle inside the API.
| await api.waitUntil("blockListFetched"); | ||
| this.checkIfContactBlocked(api.blocking.blocklist()); | ||
| api.listen.on('blockListUpdated', this.checkIfContactBlocked, this); | ||
| } else this.set({'contact_blocked': undefined}); |
| this.initialized.resolve(); | ||
| }, | ||
|
|
||
| checkIfContactBlocked (jid_set) { | ||
| if (jid_set.has(this.get('jid'))) | ||
| return this.set({'contact_blocked': true}); |
There was a problem hiding this comment.
As a general rule, we always use curly brackets for if statements, unless it's a single if (no else) one a single line.
| @@ -1104,7 +1120,7 @@ const ChatBox = ModelWithContact.extend({ | |||
| } else if ( | |||
| this.isHidden() || ( | |||
| pluggable.plugins['converse-blocking'] && | |||
| api.blockedUsers()?.has(message?.get('from_real_jid')) | |||
| api.blocking?.blocklist().has(message?.get('from_real_jid')) | |||
There was a problem hiding this comment.
This will have to be updated once api.blocking.blocklist() is async.
|
@udanieli I think I've figured out how we can run the CI tests against your code. You'll have to make a new branch in this repo (and not in your fork of this repo) and then make a new pull request. Then, I can manually trigger a tests workflow against that branch. I however can't do it for branches that are not in this repo, like your fork. https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch I think it would be better to first address the review comments, before we try that. |
|
@udanieli: Have you progressed on it? |
Hello @jcbrand, I was working on this feature and saw your branch. I did a little fix/refactoring on it, if you are interested here it is.
sendBlockingStanza()for block and unblock;trueinhandleBlockingStanza()for handler to be invoked again;Array.forEach()to avoid garbage strings in<block>and<unblock>elements.In
handleBlockingStanza()if you don't returntrue, the handler won't be invoked again.Hope this is useful.
What about making chat window readonly to avoid sending messages to blocked contacts and receiving this error from the server?
