WIP: Block users

[udanieli]

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.

• fixed buttons aesthetics and triggered a modal close after API calls;
• removed duplicate code: use sendBlockingStanza() for block and unblock;
• return true in handleBlockingStanza() for handler to be invoked again;
• changed Array.forEach() to avoid garbage strings in <block> and <unblock> elements.

In handleBlockingStanza() if you don't return true, 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?

[udanieli]

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.

[jcbrand]

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

[udanieli]

Ok squashed.

I have also added a blocking-views plugin to display blocked users in the controlbox instead of the profile

Should I add that commit to this PR?

Maybe I am missing translations too. Should I update the .pot file and all the .po ones?

[jcbrand]

I have also added a blocking-views plugin to display blocked users in the controlbox instead of the profile

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.

Should I add that commit to this PR?

I think putting it in a separate PR would keep things cleaner and more manageable.

Maybe I am missing translations too. Should I update the .pot file and all the .po ones?

You can run make po && make po to regenerate pot and po files, but I wouldn't do it as part of this PR because it will add lots of new files to the diff.

It would be best to do it after the merge.

[jcbrand]

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.

[udanieli]

Hi @udanieli, you've done great work so far, thank you!

Thank you for sharing Converse with the community.

How are you feeling so far?

Are you enjoying working on Converse and motivated to continue? Or are you becoming tired?

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 pubsub plugin with the subscribe() method and a listener for the notifications.

I have more feedback on this PR, but I want to be respectful of your time.

All your reviews are welcome.

[jcbrand]

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

[Neustradamus]

@udanieli: Have you progressed on it?

[jcbrand]

Closing in favor of #3574