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

Share an email thread to workspace members chip and dropdown (#4199) #5640

Merged
merged 33 commits into from
Jul 31, 2024
Merged

Share an email thread to workspace members chip and dropdown (#4199) #5640

merged 33 commits into from
Jul 31, 2024

Conversation

pereira0x
Copy link
Contributor

Feature: Email thread members visibility

For this feature we implemented a chip and a dropdown menu that allows users to check which workspace members can see an email thread, as depicted on issue (#4199).

Implementations

  • create a new database table (messageThreadMember)
  • relations between messageThreadMembers and the relevant existing tables (MessageThread and WorkspaceMembers)
  • added a new column to the MessageThread table: everyone - to indicate that all workspace members can see the email thread
  • create a new repository for the new table, including new queries
  • edit the queries so that the new fields could be fetched from the frontend
  • created a component MultiChip, that shows a group of user avatars, instead of just one
  • created a component, ShareDropdownMenu, that shows up once the EmailThreadMembersChip is clicked. On this menu you can see which workspace members can view the email thread.

Screenshots

Here are some screenshots of the frontend components that were created:

Chip with everyone in the workspace being part of the message thread:
image

Chip with just one member of the workspace (the owner) being part of the message thread:
image

Chip with some members of the workspace being part of the message thread:
image

How the chip looks in a message thread:
image

Dropdown that opens when you click on the chip:
image

Testing and Mock data

We also added mock data (TypeORM seeds), focusing on adding mock data related to message thread members.

Conclusion

As some of the changes that we needed to do, regarding the change of visibility of the message thread, were not covered by the existing documentation, we were told to open a PR and ask for feedback on this part of the implementation. Right now, our implementation is focused on displaying who is part of an email thread.

Feel free to let us know which steps we should follow next :)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Introduced EmailThreadMembersChip component to display email thread visibility.
  • Added new fields to fetchAllThreadMessagesOperationSignatureFactory.ts for message thread members.
  • Updated RightDrawerEmailThread.tsx to manage message thread state with Recoil.
  • Created new types MessageThread and MessageThreadMember.
  • Added MessageThreadMembersBar component to show email thread members in the right drawer.

messageThread: MessageThread | null;
}) => {
const renderChip = () => {
if (!messageThread) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming isEveryone to isVisibleToEveryone for better clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't feel the need for this change, as we wanted the variable to be consistent with the database table.

Copy link
Member

Choose a reason for hiding this comment

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

I have to agree with that I also didn't understand what Everyone was

Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop everything related to "everyone" and focus on 1-to-1 sharing for v1. Maybe "everyone" should be managed through the permission layer instead. And here we'll manage something a bit more fine-grained with product implications which would be "thread subscribers" (people who would receive notifications when a new message is received for example)


export type MessageThread = {
id: string;
everyone: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming everyone to something more descriptive like isVisibleToAll for better clarity.

Copy link
Contributor

@simaosanguinho simaosanguinho Jun 4, 2024

Choose a reason for hiding this comment

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

We think everyone is fine for this case.

@Bonapara
Copy link
Member

Thanks so much @pereira0x! From a design perspective, there is an unwanted top bar on the following component:

Dropdown that opens when you click on the chip: image

@simaosanguinho
Copy link
Contributor

Hey @Bonapara. Me and @pereira0x will take care of removing the unwanted top bar from the Dropdown component. Additionally, we'll review the bot suggestions and apply them to our patch.

@simaosanguinho
Copy link
Contributor

Good afternoon everyone! Just pushed some changes, please tell us if we missed something.

@simaosanguinho
Copy link
Contributor

Hi everyone. Regarding shared emails visibility change, me and @pereira0x were told on Discord that we could not leverage useFindManyRecords to change the visibility on shared email threads, given that nested query filters are not supported yet. Any ideas how we can move forward now @charlesBochet?

@FelixMalfait
Copy link
Member

Hey thanks for this and apologies for the slow review! We'll review and try to get it merged this week.
Do you think you can handle the merge conflicts? I know it's a pain sorry

@simaosanguinho
Copy link
Contributor

No worries!! The merge conflicts will be solved by the end of tomorrow.

pereira0x and others added 19 commits June 29, 2024 12:32
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: José Pereira <jose.a.pereira@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
Co-authored-by: Simão Sanguinho <simao.sanguinho@tecnico.ulisboa.pt>
@pereira0x
Copy link
Contributor Author

pereira0x commented Jun 29, 2024

Hey, just letting you guys know that the new conflicts are now fixed!

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Hey apologies it took months to review!!! That's great work you did here

@@ -0,0 +1,132 @@
import { useTheme } from '@emotion/react';
import { offset } from '@floating-ui/react';
import {
Copy link
Member

Choose a reason for hiding this comment

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

Could you hide the frontend logic behind a feature flag? That would allow merging this intermediate PR before we do a second one

@@ -0,0 +1,78 @@
import { useTheme } from '@emotion/react';
Copy link
Member

Choose a reason for hiding this comment

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

Every component should have a storybook story :)

})
@WorkspaceIsNotAuditLogged()
@WorkspaceIsSystem()
export class MessageThreadMemberWorkspaceEntity extends BaseWorkspaceEntity {
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this object to MessageThreadSubscriber?

import { WorkspaceDataSourceService } from 'src/engine/workspace-datasource/workspace-datasource.service';

@Injectable()
export class MessageThreadMemberRepository {
Copy link
Member

Choose a reason for hiding this comment

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

FYI We've now introduced TwentyORM which allows doing this elegantly. We're trying to cleanup the code base from sql raw query (but still a LOT of work ahead)

MESSAGE_THREAD_1: '20202020-8bfa-453b-b99b-bc435a7d4da8',
MESSAGE_THREAD_2: '20202020-634a-4fde-aa7c-28a0eaf203ca',
MESSAGE_THREAD_3: '20202020-1b56-4f10-a2fa-2ccaddf81f6c',
MESSAGE_THREAD_4: '20202020-d51c-485a-b1b6-ed7c63e05d72',
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any message in my local database for 20202020-d51c-485a-b1b6-ed7c63e05d72, did I miss anything?

}

return (
<Chip
Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2024-07-02 at 15 00 01

It didn't work for me. Are there 4 or 5 results?

@charlesBochet charlesBochet mentioned this pull request Jul 11, 2024
@lucasbordeau lucasbordeau merged commit c3417dd into twentyhq:main Jul 31, 2024
5 of 9 checks passed
Copy link
Contributor

Thanks @pereira0x for your contribution!
This marks your 1st PR on the repo. You're top 20% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants