search

okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
331 stars 44 forks source link

DMs are not displayed per-group #1636

Closed taoeffect closed 9 months ago

taoeffect commented 1 year ago

Problem

DMs are not per-group as seen in this video:

https://github.com/okTurtles/group-income/assets/138706/53477a49-6181-4735-8697-10ae877f1af4

Reproduce:

  1. u1 create a group (g1)
  2. u1 invite u2, and u2 join (in a separate container tab)
  3. u1 invite u4, and u4 join (in a separate container tab)
  4. u1 create DM with u2 in g1
  5. u3 sign up (in a separate container tab) create a new group (g2)
  6. u3 copy invite link and invite u1 to join g2
  7. u1 now part of g1 and g2, switch between them, see DM between u1 and u2 visible in both groups

Solution

Find and fix the getters that are not scoped to per-group properly.

Silver-IT commented 1 year ago

I thought it should not be per group just like discord. Please check the summary of the PR #1443.

taoeffect commented 1 year ago

@Silver-IT Sorry for the confusion! By "like discord" I meant that we'll have a special group that's outside of all groups, see #1619. Within groups we should still have DMs map to DMs between group members only.

So this doesn't mean you should delete the existing getters, as those will come in handy in the future, but rather create new group-specific getters.

Silver-IT commented 1 year ago

Thanks for your description. By the way, how can we figure out which DM is per group and which DM is not per group? I mean in terms of UX.

Silver-IT commented 1 year ago

Aha, that design should be from Issue #1619. Right?

taoeffect commented 1 year ago

By the way, how can we figure out which DM is per group and which DM is not per group? I mean in terms of UX.

When you're in the group, and you look at the navbar on the right, that shows you per-group DMs.

When you're in the group dashboard, that will have its own UI for DMs that's different.

Aha, that design should be from Issue #1619. Right?

Yes, that is what I mean by "will have its own UI" above.

taoeffect commented 1 year ago

Adding Note:On Hold and Note:Discussion to this, because we need to avoid creating two types of DMs in the codebase (and also wait for e2e-protocol to be merged).

@Silver-IT Is there a way maybe to show DMs in the list that are between group members only, without creating two different types of DMs?

For example, you could have one getter that returns all DMs (allDMs), and another getter called currentGroupDMs that takes the result of allDMs and filters for DMs that are only between group members of the current group?

Silver-IT commented 1 year ago

Sure. Why not? In fact, mailbox contract has it's getter to return global DMs and group contract also has it's getter to return the group DMs. This is the reason why I wanted to know the best two names for these two.

Also we can filter state.contracts and get only chatroom contracts with it's type (CHATROOM_TYPES.INDIVIDUAL) to get all DMs not using the above two getters.

taoeffect commented 1 year ago

OK, yeah, if the mailbox contract is involved in this, then we should wait to work on this issue until #1521 is merged, because @corrideat has deleted mailbox and moved everything from there into identity

Silver-IT commented 1 year ago

Okay. I never knew that the mailbox contract is being burnt away. :)

Silver-IT commented 9 months ago

I think this issue should be closed thanks to the recent updates regarding the handling chatrooms. ourGroupDirectMessages is used in ChatMembers.vue, and it only returns the direct messages which are created inside the group.

@taoeffect, you can close this issue after testing this in the latest master branch.

taoeffect commented 9 months ago

Good catch @Silver-IT - you're right this has been fixed!