alphapapa
Add: (ement-room-kick-user, ement-room-ban-user, ement-room-unban-user) New commands
All done for now. Tested, and looking good, I think (for an initial version, at least).
Some comments of note from the testing:
It seemed to work pretty well from this side too: the ban message showed up in the room buffer, and the was moved into the [Left] group. When you unbanned me, the unban event showed up in the room buffer too.
Rejoining was a bit more work, because I had to get the room ID manually instead of using completion, but it worked once I was invited.
The other thing we might consider is to implement a convenience feature to also remove all recent events by the banned user, like Element offers to do when you ban someone. Helps with removing spam.
Can always delete the messages manually in the interim, though.
Interesting side effect: I killed the room buffer and reopened it so I could see and accept the invitation, then after rejoining, the events in the room that I already had are duplicated. I guess the server thinks we don't already have those events, so it sends them again, but we don't delete them, so we end up with multiple copies. Although I thought we already had code to deduplicate events…
I see it's possible for the prompt to come up as "Ban user nil <@spammer123:matrix.org> ...?", so I should account for them not having a display name.
I see it's possible for the prompt to come up as "Ban user nil <@spammer123:matrix.org> ...?", so I should account for them not having a display name.
We should probably just add a convenience function to format users for display in these cases, e.g.
(cl-defun ement--format-user-id (user &key with-id-p (room ement-room))
"Return string describing USER.
If ROOM, return the user's displayname in that room, when set;
otherwise, the user's global displayname. If WITH-ID-P, quote
the displayname and include the user's MXID, like an email
address."
(let ((displayname (if room
;; FIXME: If a membership state event has not yet been received, this
;; sets the display name in the room to the user ID, and that prevents
;; the display name from being used if the state event arrives later.
(ement--user-displayname-in room user)
(or (ement-user-displayname user)
(ement-user-username user)))))
(if with-id-p
(if (equal displayname (ement-user-id user))
;; User has no displayname: just return the ID.
(format "<%s>" displayname)
(format "%S <%s>" displayname (ement-user-id user)))
displayname)))
By the way, I think we should add support for reporting messages to the homeserver admin. In Element, when I'm deleting spam, my procedure is usually: 1. Report to homeserver admin; 2. Ban user, 3. delete events.
I didn't know that was a thing.
https://spec.matrix.org/latest/client-server-api/#reporting-content
https://spec.matrix.org/latest/client-server-api/#reporting-content
Any thoughts on the "score" value when reporting?
A score ranges from -100 (worst/offensive) to zero (best/inoffensive), but I was finding this slightly awkward/ambiguous for UI purposes.
I decided to see what kind of UI Element presented for it, and... it doesn't. There's no score option at all. I used it to send a retrospective report for the last spam event, and the HTTP request contained a score of -100 in the JSON.
In the absence of any universally-agreed scale/mapping for the level of offence any given sort of post should warrant, "everything is -100" is certainly a simple solution.
We could provide a handful of pre-determined named values, but would need names like "Most offensive" down to "Inoffensive" rather than names like "Spam", as the latter would just be imposing our definitions of how offensive any given thing was.
We could have a very short labelled list like this, and default to... maybe -100, but maybe leaving space for something worse-than-normal?
-100 (most offensive)
-75 (very offensive)
-50 (quite offensive)
-25 (slightly offensive)
0 (inoffensive)
And even then, the labels on everything but the first and last items are quite subjective.
Thoughts?
I appreciate your thoughtfulness, but I think we should just do what Element does here. If the official client doesn't care about offering a way to set the score parameter, then we probably shouldn't bother with it. (The score parameter was probably one of those things that seemed like a good idea at the time, but as you've aptly shown, doesn't map very well to reality and usability; the same content might be -100 HIGHLY OFFENSIVE to one person and -10 MEH to another and +1,000 WINS THE INTERNET to someone else.)
I've gone with "mimic Element by default" (sending -100 automatically without asking the user), but I've left the ability to select a score if a prefix argument is used. It's a little bit configurable in case anyone really wants to change the default but, on the basis that probably no one will do that, I left that pretty minimal.
@phil-s Thanks for your work and your patience. I pushed some "Minor changes" and rebased. Let me know if you agree that this is ready to merge.
Fair enough ditching the completing-read for score. I knew that seemed slightly mad by default, but I'd kept it so that users could change the list of options to a short list and then select from a handful of choices. I'm not fussed about losing that, though.
The refactor has lost the support for reporting without a score, though. Having -100 as a pre-populated input rather than "what you'll get by default" meant the user could submit an actual empty value, and we could then omit the score from the API request. With the new code you can't do that (and the documented nil behaviour for ement-room-report-content-score-default no longer applies, as read-number can't cope with that).
I think we need to revert to reading a string in order to support this.
Fair enough ditching the completing-read for score. I knew that seemed slightly mad by default, but I'd kept it so that users could change the list of options to a short list and then select from a handful of choices. I'm not fussed about losing that, though.
I feel like that's a case of "YAGNI." :)
The refactor has lost the support for reporting without a score, though. Having
-100as a pre-populated input rather than "what you'll get by default" meant the user could submit an actual empty value, and we could then omit the score from the API request. With the new code you can't do that (and the documentednilbehaviour forement-room-report-content-score-defaultno longer applies, asread-numbercan't cope with that).I think we need to revert to reading a string in order to support this.
I checked the API, and it doesn't list the score as an optional field, so unless I'm missing something, I think we shouldn't submit it without one.
I'd figured everything not labelled Required was optional?
Well, I guess you're right. :) Would you mind testing to see what happens if we send a request without the reason and score fields, just to be certain? (I'd suggest sending it in the testing room we have, and then redacting the reported event; I think the matrix.org homeserver admins will forgive us for one unnecessary report since we're developing a client and the docs are a bit ambiguous about it.)
I reported a test message with no score, and I got a 200 response and the "Content reported." message.
I was tracing the plz call and confirmed it only had :body "{\"reason\":\"Testing. Please ignore.\"}" for the request body.
Edit: Whoops, you asked for no reason either. One moment...
Heh, ok, need to avoid this:
Error running timer `plz--respond': (ement-api-error "400: Content must be a JSON object.")
I believe this is good, now. I'm not seeing any errors from plz in any of the test scenarios. In each case I get the following from tracing plz:
(plz post "https://matrix-client.matrix.org/_matrix/client/r0/rooms/<roomid>/report/<eventid>?" :headers (("Authorization" . "Bearer <token>") ("Content-Type" . "application/json")) :body "<body>" :body-type text :as json-read :then #f(lambda (_data) [t] (message "Content reported.")) :else ement-api-error :connect-timeout 10 :timeout 60 :noquery t)
Where <body> is one of the following, depending on which optional arguments are supplied:
"{}""{\"reason\":\"Testing. Please ignore.\"}""{\"score\":0}""{\"score\":-10}""{\"score\":0,\"reason\":\"Testing. Please ignore.\"}"
Untested as yet, but I think we want this as well, so I'll just drop it in here for now.
(defun ement-room-report-delete-ban (event room session reason)
"Report the current message, delete it, and ban the sender from the room."
(interactive
(ement-room-with-highlighted-event-at (point)
(let ((event (ewoc-data (ewoc-locate ement-ewoc))))
(unless (ement-event-p event)
(user-error "No event at point"))
(if (yes-or-no-p "Report, Delete, and Ban? ")
(let ((reason (string-trim
(read-string "Reason: "
nil ement-room-report-content-reason-history nil
'inherit-input-method))))
(list event ement-room ement-session reason))
;; Aborted.
(let ((debug-on-quit nil))
(signal 'quit nil))))))
;; Insist on a reason for this particular command.
(when (string-empty-p reason)
(user-error "Must supply a reason."))
;; Do all the things.
(let ((user-id (ement-user-id (ement-event-sender event)))
(room-id (ement-room-id room))
(score ement-room-report-content-score-default))
(ement-room-report-content event room session reason score)
(ement-room-delete-message event room session reason)
(ement-room-ban-user user-id room-id session reason)))
Thanks, we definitely need some kind of "do all the things" combo command. I think it'd be best to follow Element's lead here, i.e. when banning a user, maybe a prefix argument or two could be used to indicate that we want to also delete the user's recent content. So the workflow might be like: 1) report individual events that need reporting; 2) call the "ban user" command with a prefix argument, which would present a confirmation prompt like, "Ban user and delete user's recent events?". What do you think?
In the long run, it'd be nice to have a way to mark multiple events in a room buffer, and then call a command to act on them (e.g. if not all of a user's recent events should be reported). I have some ideas about how to facilitate that in the EWOC-based view. Let me know if you have any thoughts about that.
I hadn't thought about marking messages, but I had pondered iterating over all of that sender's messages and prompting the user to report/delete them all.
What about a search loop, highlighting each message from that sender in the room, and asking y-or-n-p for whether to add it to the list of messages to deal with? That might be relatively easy to implement.
I guess we could use (ement-room-occur :user-id sender) to bring up a buffer with just their messages, and make the selections from there?
Yeah, I like that idea. (Not expecting you to code it, necessarily, but feel free to if you are interested.)
I've pushed a command ement-room-report-delete-ban-selected for feedback.
You'll note that at present it doesn't issue the actual report/delete/ban requests (which have been pretty well-tested at this point, so ought to work). I've commented those lines out from this new function, and instead it just issues a message for each action, so that people can play with the UI and see if they like it.
Please note that all of the other commands in this PR do perform their actions, including the earlier ement-room-report-delete-ban (which acts only on the message at point).
I had an opportunity to use the new command in practice today (after I removed the training wheels, locally), and it worked fine.
I'm still interested in feedback on the UI (including whether it should even be a separate command in the menu, or just rolled into the "ban a user" command).
Hi @phil-s, I rebased on current master and then pushed https://github.com/alphapapa/ement.el/commit/8b853d852c73e732ce95b5491e475a9aaf97b115. Please let me know what you think about that. If you concur, then I think we should go ahead and merge this.
I'm still interested in feedback on the UI (including whether it should even be a separate command in the menu, or just rolled into the "ban a user" command).
I don't have a strong opinion about it yet, but I think the important thing is that, if the user does one action before another (like banning before reporting/redacting content), it should still be easy to do all the things; it shouldn't depend on remembering which command to use first. Does that make sense?