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

feat(groupChatManager): create a groupChat manager to create separate collections on a thread #1291

Merged
merged 13 commits into from
Feb 10, 2022

Conversation

Jekrimo
Copy link
Contributor

@Jekrimo Jekrimo commented Feb 2, 2022

What this PR does 📖
create a groupChat manager to create separate collections on a thread, allow sending messages, listening to the collection for changes, eventEmitters for testing I presume

Which issue(s) this PR fixes 🔨

AP-660

Special notes for reviewers 🗒️
EDIT: Testable now. 3 Groups are hardcoded for testing. Users will show as unknown unless it's your user. There are also other bug's and features missing currently. Most have been identified in AP-732 -> AP-744

@Jekrimo Jekrimo requested a review from iltumio February 2, 2022 21:03
@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 2, 2022
libraries/Textile/GroupChatManager.ts Outdated Show resolved Hide resolved
libraries/Textile/GroupChatManager.ts Outdated Show resolved Hide resolved
@iltumio iltumio added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Feb 3, 2022
@stavares843 stavares843 changed the title feat(groupChatManager): create a groupChat manager to create seperate collections on a thread feat(groupChatManager): create a groupChat manager to create separate collections on a thread Feb 3, 2022
@phillsatellite
Copy link
Contributor

phillsatellite commented Feb 3, 2022

Tested: I'm not 100% sure if its caused by the PR but can't seem to get past the Linking Satellites page when creating a new account. I switched back to Dev branch, created new account and got in fine, switched back to this branch and created new account and still got blocked at Linking Satellites
Screen Shot 2022-02-03 at 2 23 10 PM

@Jekrimo Jekrimo added the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Feb 4, 2022
@stavares843 stavares843 removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 7, 2022
@Jekrimo Jekrimo removed the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Feb 8, 2022
@Jekrimo Jekrimo requested a review from iltumio February 8, 2022 15:16
@Jekrimo Jekrimo added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 8, 2022
@stavares843
Copy link
Member

/rebase

@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Feb 8, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Feb 9, 2022
@github-actions github-actions bot removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Feb 9, 2022
@stavares843 stavares843 removed the QA tested One QA Person has tested - Needs QA Lead review still label Feb 9, 2022
@stavares843
Copy link
Member

tickets that jeff added

AP-732
AP-733
AP-734
AP-735
AP-736
AP-737
AP-738
AP-739
AP-740
AP-741
AP-742
AP-743
AP-744

thanks for adding @Jekrimo 🔨

@phillsatellite
Copy link
Contributor

Tested: Only issues I've found that I don't think weren't already addressed was the error in terminal (switched to branch, yarn, yarn dev to get that error) and the other error in Console "Error: errors.textile.friend_not_found"
Screen Shot 2022-02-09 at 4 49 06 PM

Screen Shot 2022-02-09 at 4 41 09 PM

@phillsatellite phillsatellite added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 9, 2022
@Jekrimo
Copy link
Contributor Author

Jekrimo commented Feb 9, 2022

Tested: Only issues I've found that I don't think weren't already addressed was the error in terminal (switched to branch, yarn, yarn dev to get that error) and the other error in Console "Error: errors.textile.friend_not_found" Screen Shot 2022-02-09 at 4 49 06 PM

Screen Shot 2022-02-09 at 4 41 09 PM

I think this has to do with us tricking it currently to allow multi users. I say we leave it and open a bug for it. But Idk what everyone else thinks

@stavares843
Copy link
Member

do we know why appears the errors.textile.friend_not_found from Phil's screenshot? I see that added here
https://github.com/Satellite-im/locales/blob/main/en-US.js#L469

can we fix the warning when it finishes compiling locally? atm we have zero compiling warnings after compiling

@iltumio iltumio removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 10, 2022
@netlify
Copy link

netlify bot commented Feb 10, 2022

✔️ Yeeeehaw, deploy preview is ready!

🔨 Explore the source changes: fee27c5

🔍 Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/6205176d3dd85a0008742882

😎 Browse the preview: https://deploy-preview-1291--adoring-edison-dbcef8.netlify.app

@stavares843
Copy link
Member

is possible to get the textile error that phil mentioned above fixed as well? 🔨

@Jekrimo
Copy link
Contributor Author

Jekrimo commented Feb 10, 2022

I'm tryin to recreate it and can't get the error, is it showing for anyone else?

@phillsatellite
Copy link
Contributor

I'm tryin to recreate it and can't get the error, is it showing for anyone else?

Hey Jeff! Sorry didn't notice earlier I got that error from trying to react to a message, which you said above isnt working yet 👍

@stavares843
Copy link
Member

thanks for testing @phillsatellite

@phillsatellite phillsatellite added the QA tested One QA Person has tested - Needs QA Lead review still label Feb 10, 2022
@Jekrimo
Copy link
Contributor Author

Jekrimo commented Feb 10, 2022

Awesome! That's great news, perfect!

if (
RegExp(this.$Config.regex.uuidv4).test(this.recipient.textilePubkey)
) {
this.$store.dispatch('textile/sendGroupMessage', {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about dumping the if/else and doing something like

        // Check if the message is a group or a direct message
        const messageType = RegExp(this.$Config.regex.uuidv4).test(
          this.recipient.textilePubkey,
        )
          ? 'sendGroupMessage'
          : 'sendTextMessage'

        this.$store.dispatch(`textile/${messageType}`, {
          to: this.recipient.textilePubkey,
          text: value,
        })
        ```

@@ -4,6 +4,7 @@ import Vue, { PropType } from 'vue'
import { mapState } from 'vuex'

import { MessageGroup } from '~/types/messaging'
import { Message } from '~/types/textile/mailbox'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this getting ready for something in the future, or leftover

@WanderingHogan WanderingHogan removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 10, 2022
@stavares843 stavares843 merged commit 489f188 into dev Feb 10, 2022
@stavares843 stavares843 deleted the AP-660 branch February 10, 2022 21:58
@github-actions github-actions bot removed the QA tested One QA Person has tested - Needs QA Lead review still label Feb 10, 2022
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.

None yet

5 participants