[Comment] Mention @discussion or @group to represent all members of the thread or group #10934

jerinkc · 2024-07-03T11:54:21Z

Mentioning @discussion in the comment will notify all members of the discussion.
Mentioning @group in the comment will notify all members of the group.

Closes #8148

…) and corresponding changes to model

…ice_spec

Edit to group_service_spec

…threads

… changes related to mentioning thread with the help of discussion

Comment Spec

…us changes of adding collections as a member to group

Remove forbidden name validation for username comment_spec update Name change: COLLECTIONS -> MENTIONABLE_COLLECTIONS

robguthrie · 2024-07-04T23:43:48Z

Hi @jerinkc - it's really wonderful to see you've made a PR to start on this feature. Thank you. I've not seen a loomio feature PR in years.

With the greatest respect, I don't think it's going in the right direction currently - I can understand why, this is a super challenging feature to add, not only is the existing mentioning code complex because we support both markdown and html comment formats, it has some legacy behavior support included in it too.

Some tips I can offer:

have a look at app/services/announcement_service.rb we have the key words that reference group and discussion_group (thread) in there for when you invite people to a discussion thread.
have a look at app/models/concerns/has_mentions.rb and make a new file app/models/concerns/has_audience_mentions.rb copying the style in has_mentions but looking for 'group' and 'discussion_group' rather than usernames.
Then create a clone of app/models/concerns/events/notify/mentions.rb called .../notify/audiences.rb which looks for mentioned_audiences and sends app/models/events/comment_announced.rb (also a new file)

Does that make any sense? You're free to explore the approach however you like, but this is how I recommend you look at it

…e_collection.rb

…nges

jerinkc · 2024-07-05T16:43:13Z

Hey @robguthrie ,

Thank you for your quick review and insightful comment on the current approach. I genuinely appreciate your feedback!

I’ll take the suggested changes into account and try to implement them.

jerinkc · 2024-07-07T14:47:08Z

Hey @robguthrie,

Thank you once again for the tips! The approach is perfectly clear to me. I want to get your opinion on a couple of additional points:

In your previous comment, you mentioned ‘discussion_group’ instead of ‘thread.’ Should I use the term ‘discussion_group’ instead of ‘thread’ throughout the implementation?
Regarding the current approach, I’ve used two users to represent the audience in the database. I find this helpful when listing users for mentions in the UI. If this approach is acceptable, I plan to use these users to store notifications for audiences.

robguthrie · 2024-07-07T23:14:29Z

Hi @jerinkc

On the server side we tend to use the term discussion rather than thread. discussion_group or discussion would be best.
I don't think you should continue with the approach of pushing this through a fake user account. It would be better to list "audiences" with users when mentioning, then with the separate has_audience_mentions concerns you detect the use of the discussion or group token. The has_mentions concern will just ignore this.

I suggest you ignore the frontend stuff for now. Write some tests that create a comment with a <span data-mention-id="group"> in them, then use that to build the has_audience_mention concern, which creates it's own notification type. It will share a similar pattern to has_mentions but be parallel to it, and the has_mentions probably won't need to change.

jerinkc · 2024-07-30T12:41:13Z

Hey @robguthrie,

I have added more changes along with the internationalization feature.

Also, should I work with the UI part here or would you prefer it in another PR?

spec/models/comment_spec.rb

app/models/audience.rb

robguthrie · 2024-07-30T21:21:21Z

You can put the UI stuff in here too, please.

Sorry there was a misunderstanding about how to handle translation. The code/server side should just be english in all cases - it will be surprisingly simple to translate just the visible text with the existing translation library in the UI only.

jerinkc · 2024-07-31T07:02:45Z

@robguthrie

I understand you want to apply translation only to the visible part of the UI. Which is for the UI code it should be something like,

<p>Hey, 
    <span class=\"mention\"
        data-mention-id=\"discussion\"
         label=\"discussion\">
         @diskussion
    </span></p>

So the server will recognize when a discussion is mentioned.

If internationalization applies to the markdown format, how will it detect that the incoming message has mentioned audiences without the current configuration? I think we should retain the current translating functionality(including back_translate method) and only apply it to the markdown format. What do you think?

robguthrie · 2024-08-02T04:10:39Z

Oh Yeah. That's right, it will be necessary for markdown formatted content. Thanks for thinking of that.

…changes

backend - accept translated HTML content with translated attributes

Minor translation changes Test with locale 'fe' -> 'fr'

Modify Frontend translation functionality

…body" This reverts commit c55cdc7.

jerinkc · 2024-08-22T05:11:41Z

@robguthrie Take a look whenever you get a chance. Thanks!

robguthrie · 2024-09-05T02:37:26Z

Sorry I've taken so long to review this. Will run and test it tomorrow.
Can you tell me - do you think this is finished? Do you think there is any more work to do, and have you had the opportunity to deploy it and test it with users?

jerinkc · 2024-09-06T17:22:48Z

Hey,

It is done if it applies only to comments. I’ve tested it locally with multiple users, and everything is working smoothly.

Note: Translations for other languages are not included.

Here’s a quick demo: Video Link

robguthrie · 2024-09-09T03:06:23Z

config/locales/client.en.yml

@@ -714,6 +718,7 @@ en:
      user_added_to_group: "%{actor} added you to the group: %{title}"
      user_mentioned: "%{actor} mentioned you in: %{title}"
      user_reminded: "%{actor} is reminding you to vote in: %{title}"
+      comment_announced: "%{actor} mentioned you in: %{title}"


I wonder if this should be more specific. What do you think about having two messages:
"%{actor} notified everyone in the group" and
"%{actor} notified everyone in the thread"

robguthrie · 2024-09-09T03:36:07Z

Ok, I've given this a test run. It looks really promising! Well done.

I notice that you need to type @discussion to mention everyone in the thread.. The Loomio term is "thread" so I think it would make sense to use that as the keyword.

Also when I check the "Notified" button I see this:

Which is from notifying the group.. but there are no entries for notifying the thread. I'm not sure why that is.

I'll look more closely at it and get back to you

discussion -> thread in UI

…otification_serializer

jerinkc · 2024-09-12T08:02:20Z

I missed the notification part during implementation, it has been added in the latest commits. Also, the UI now uses @thread instead of @discussion.

robguthrie · 2024-09-15T22:34:59Z

Wow. It's really coming together now. There are two last things that need some attention..

Permissions.

Frontend:
Can you ensure that @thread and @group mention suggestions only show when the user is permitted to notify?
We need to call AbilityService.canNotifyGroup and AbilityService.canAnnounceDiscussion before adding the mention options to the list and presenting them to the user.

https://github.com/loomio/loomio/blob/master/vue/src/shared/services/ability_service.js#L135C3-L135C24

Backend:

To be secure we also need to authorize this on the back end. You can see an example of this here:

https://github.com/loomio/loomio/blob/master/app/extras/user_inviter.rb#L72

So you can call user.ability.authorize! :announce, discussion and user.ability.authorize! :announce, group on the back end to ensure the client is allowed to notify.

Education:
It would be good to explain what these options do, as people will see them and not know.. I wonder if we can put a subtitle in the suggestions list "Notify everyone in the group" under the @group, for example.

jerinkc · 2024-10-13T08:59:28Z

Hey, I have made the necessary changes. Also, I used user.ability.authorize! :notify, group instead of user.ability.authorize! :announce, group. Let me know if it works.

robguthrie · 2024-10-13T10:05:11Z

Will do! Thanks!

…

On Sun, Oct 13, 2024 at 9:59 PM Jerin ***@***.***> wrote: Hey, I have made the necessary changes. Also, I used user.ability.authorize! :notify, group instead of user.ability.authorize! :announce, group. Let me know if it works. Screenshot.2024-10-13.at.12.58.25.PM.png (view on web) <https://github.com/user-attachments/assets/dfd7ba2b-5a9b-46d6-8377-ef39bce29fa1> Screenshot.2024-10-13.at.12.42.20.PM.png (view on web) <https://github.com/user-attachments/assets/7c36a52e-021d-4af9-9780-febf86acc685> — Reply to this email directly, view it on GitHub <#10934 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADWXX36CAC7EFUTCVU6ZUTZ3IZALAVCNFSM6AAAAABKJNGPNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYHA4TCNJXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jerinkc added 21 commits July 3, 2024 16:43

Added column collection representing collection of users(group/thread…

2a0791b

…) and corresponding changes to model

Add group_collection as a member and remove from members list

8a52f0f

Update specs for collection users - memberships_controller, poll_serv…

28b051d

…ice_spec

Update to mention 'thread' and spec change

8af9e6d

Edit to group_service_spec

Prevent collection user from invite modal listing

5e482eb

Prevent user collections from including members count for groups and …

2ae10d0

…threads

Add mentionable user collection on group creation and spec changes

274361c

Remove changes from commit 584e313b0334ca8a6c694dcfa0d6c65b3e4a94bd -…

1c87f41

… changes related to mentioning thread with the help of discussion

Return mentioned users and collections from comment

5a93a3b

Comment Spec

Store user collections in cache

25404b5

Fix comment_spec

3b3c9f3

Approach change - Independent mentionable collections - Remove previo…

7a9a351

…us changes of adding collections as a member to group

validation name change: forbidden_name_user_name -> forbidden_name

dfe1be0

Remove forbidden name validation for username comment_spec update Name change: COLLECTIONS -> MENTIONABLE_COLLECTIONS

Remove collection users create on comment spec file

cd4cedb

Migration file to input collections to database

4f6fae4

Remove author from mentioned users

30a546f

Add collection users on development setup

d579e37

Update mentioned mentioned_users method query

9e22241

Comment spec update

022be38

Bug fixes form current branch

845b51e

Remove unused method collection_#{name}

34f3294

jerinkc changed the title ~~Mention @thread or @group to represent all members of the thread or group~~ [WIP] Mention @thread or @group to represent all members of the thread or group Jul 3, 2024

Display error on frontend and translation(en) for name error

d1e7db9

jerinkc added 3 commits July 5, 2024 19:29

Remove migration to create collections and moved to helper mentionabl…

90f5981

…e_collection.rb

Changes to choose MentionableCollection to get records

ee43d86

Rename existing usernames same as the collections and other minor cha…

05e7acc

…nges

jerinkc added 2 commits July 30, 2024 17:32

reserved name translation error for de, fr

f6d6c7d

User spec file change - check validation with error name

a44ff73

robguthrie reviewed Jul 30, 2024

View reviewed changes

spec/models/comment_spec.rb Outdated Show resolved Hide resolved

robguthrie reviewed Jul 30, 2024

View reviewed changes

app/models/audience.rb Show resolved Hide resolved

jerinkc added 8 commits August 2, 2024 20:32

Function back_translate only apply to md content and minor spec file …

88cad87

…changes

UI - mentions, added client translations

b6d92c8

backend - accept translated HTML content with translated attributes

Spec change - translated audience in data-mention-id of HTML body

c55cdc7

Get audience translation with exception throw

cd0afe0

Minor translation changes Test with locale 'fe' -> 'fr'

Modify has_audience_mentions->mentined_audience funciton

16043bd

Modify Frontend translation functionality

Revert "Spec change - translated audience in data-mention-id of HTML …

bb8c1d8

…body" This reverts commit c55cdc7.

Replace all_translated with parameterized all

825cdbb

Bugfix - back_translate returns array if non-audience given

231bf34

comment_announced translation for notification

57913f5

robguthrie reviewed Sep 9, 2024

View reviewed changes

jerinkc added 2 commits September 10, 2024 23:59

Edit to comment announced message

88e29b2

discussion -> thread in UI

comment_announced in notification history and minor modification to n…

5271d95

…otification_serializer

jerinkc added 2 commits October 11, 2024 23:57

User abilities with audience mentions

c18c310

Mention audience message

e624eca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Comment] Mention @discussion or @group to represent all members of the thread or group #10934

[Comment] Mention @discussion or @group to represent all members of the thread or group #10934

jerinkc commented Jul 3, 2024 •

edited

Loading

robguthrie commented Jul 4, 2024

jerinkc commented Jul 5, 2024

jerinkc commented Jul 7, 2024

robguthrie commented Jul 7, 2024 •

edited

Loading

jerinkc commented Jul 30, 2024

robguthrie commented Jul 30, 2024

jerinkc commented Jul 31, 2024 •

edited

Loading

robguthrie commented Aug 2, 2024

jerinkc commented Aug 22, 2024

robguthrie commented Sep 5, 2024

jerinkc commented Sep 6, 2024 •

edited

Loading

robguthrie Sep 9, 2024

robguthrie commented Sep 9, 2024

jerinkc commented Sep 12, 2024 •

edited

Loading

robguthrie commented Sep 15, 2024

jerinkc commented Oct 13, 2024

robguthrie commented Oct 13, 2024 via email

[Comment] Mention @discussion or @group to represent all members of the thread or group #10934

Are you sure you want to change the base?

[Comment] Mention @discussion or @group to represent all members of the thread or group #10934

Conversation

jerinkc commented Jul 3, 2024 • edited Loading

robguthrie commented Jul 4, 2024

jerinkc commented Jul 5, 2024

jerinkc commented Jul 7, 2024

robguthrie commented Jul 7, 2024 • edited Loading

jerinkc commented Jul 30, 2024

robguthrie commented Jul 30, 2024

jerinkc commented Jul 31, 2024 • edited Loading

robguthrie commented Aug 2, 2024

jerinkc commented Aug 22, 2024

robguthrie commented Sep 5, 2024

jerinkc commented Sep 6, 2024 • edited Loading

robguthrie Sep 9, 2024

Choose a reason for hiding this comment

robguthrie commented Sep 9, 2024

jerinkc commented Sep 12, 2024 • edited Loading

robguthrie commented Sep 15, 2024

jerinkc commented Oct 13, 2024

robguthrie commented Oct 13, 2024 via email

jerinkc commented Jul 3, 2024 •

edited

Loading

robguthrie commented Jul 7, 2024 •

edited

Loading

jerinkc commented Jul 31, 2024 •

edited

Loading

jerinkc commented Sep 6, 2024 •

edited

Loading

jerinkc commented Sep 12, 2024 •

edited

Loading