Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2002 allow DM to yourself #2343

Merged
merged 16 commits into from
Sep 12, 2024
Merged

Conversation

SebinSong
Copy link
Collaborator

@SebinSong SebinSong commented Sep 10, 2024

closes #2002

@SebinSong SebinSong self-assigned this Sep 10, 2024
@@ -117,9 +115,6 @@ export default ({
watch: {
value () {
this.$emit('change', this.value)
},
usernames () {
this.clear()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently usernames here was meant to be removed in this previous commit but went unnoticed, causing a run-time error. So removed it here.

Copy link

cypress bot commented Sep 10, 2024

group-income    Run #3081

Run Properties:  status check passed Passed #3081  •  git commit 5525afd23c ℹ️: Merge 258f046499d10e04a22ace3fe4b56dc12fae369d into 43a05a7e6c81410dbb17836aba19...
Project group-income
Branch Review sebin/task/#2002-allow-dm-to-yourself
Run status status check passed Passed #3081
Run duration 09m 21s
Commit git commit 5525afd23c ℹ️: Merge 258f046499d10e04a22ace3fe4b56dc12fae369d into 43a05a7e6c81410dbb17836aba19...
Committer Sebin Song
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111
View all changes introduced in this branch ↗︎

@SebinSong
Copy link
Collaborator Author

SebinSong commented Sep 10, 2024

@taoeffect
Let me know if you think we need to create a test-step for creating a DM to myself in group-chat-direct-message.spec.js too. I can either work on it as part of this PR or as a follow-up issue.

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @SebinSong, preliminary / partial review and one question:

Comment on lines +481 to +483
const partnerIDs = params.data.memberIDs
.filter(memberID => memberID !== rootGetters.ourIdentityContractId)
.map(memberID => rootGetters.ourContactProfilesById[memberID].contractID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change being made? I don't quite understand the reason for filtering out our own contract even if it's intentionally specified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partnerIDs here is used to call sbp(gi.actions/chatroom/join) further down the logic [2], which is called by default for the DM creator [1].

    await sbp('gi.actions/chatroom/join', { ... }) [1]

    for (const partnerID of partnerIDs) {
      await sbp('gi.actions/chatroom/join', { ... }) [2]
    }

Now that we expose the current user in this 'create new DM' modal list, there is cases where the members contain the current user too. This case needed to be taken account here.

me-in-other

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is an issue with this PR or something that went unnoticed.

  1. I created a group and invited u2 and u3.
  2. u2 created a DM with u1
  3. u1 then added u3 to that DM:

Screenshot 2024-09-10 at 6 17 10 PM

And now for u1 there's a DM with u2 and u3, but the user icon is grey:

Screenshot 2024-09-10 at 6 16 57 PM

@SebinSong
Copy link
Collaborator Author

@taoeffect Fixed the typo that caused that greyed-out photo issue. it does show now.

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent job @SebinSong! 👏

@taoeffect taoeffect merged commit ca27856 into master Sep 12, 2024
4 checks passed
@taoeffect taoeffect deleted the sebin/task/#2002-allow-dm-to-yourself branch September 12, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow creation of DM with yourself
2 participants