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: Add ability to create guest links with password (SQSERVICES-1975) #15014

Merged
merged 65 commits into from
Dec 20, 2023

Conversation

thisisamir98
Copy link
Contributor

@thisisamir98 thisisamir98 commented Apr 14, 2023

Sub-taskSQSERVICES-1975 [web] implement flow of creating a guest link with a password


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Adds a password option when using a guest link.

Causes (Optional)

Briefly describe the causes behind the issues. This could be helpful to understand the adopted solutions behind some nasty bugs or complex issues.

Solutions

Briefly describe the solutions you have implemented for the issues explained above.

Dependencies (Optional)

If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.

Needs releases with:

  • GitHub link to other pull request

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #15014 (59d4ab0) into dev (0d0ca5b) will decrease coverage by 0.09%.
The diff coverage is 33.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #15014      +/-   ##
==========================================
- Coverage   45.57%   45.49%   -0.09%     
==========================================
  Files         732      736       +4     
  Lines       23746    23964     +218     
  Branches     5405     5455      +50     
==========================================
+ Hits        10823    10903      +80     
- Misses      11549    11662     +113     
- Partials     1374     1399      +25     

@thisisamir98
Copy link
Contributor Author

We can't merge till other platforms catch up, at least they have to implement join flow for guest links with passwords!

Comment on lines 168 to 171
if (!codeOrMailInput.current) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't move it to (isFetching || !codeOrMailInput.current) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in
4cab38d

@@ -596,6 +613,11 @@
"guestRoomToggleInfoDisabled": "You can't disable the guest option in this conversation, as it has been created by someone from another team.",
"guestRoomToggleInfoExtended": "Open this conversation to people outside your team. You can always change it later.",
"guestRoomToggleInfoHead": "Guest Links",
"guestLinkPasswordModal.headline": "[Group Conversation] \n Enter password",
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 \n really working 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah if you do white-space: pre or pre-line it works.

}
}
if (isBackendError(error)) {
handleSubmitError(error as BackendError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need the as BackendError here? Seems like you have a typeguard above it should type it for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch 4 times, most recently from 1d8e6a5 to 0976347 Compare November 1, 2023 15:19
@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch from 0976347 to e8967d0 Compare November 2, 2023 10:25
@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch from ae8d5c5 to 0c84609 Compare November 9, 2023 15:39
@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch from 73818a4 to 5e70571 Compare November 9, 2023 16:29
* @param {string} password - The password to be checked.
* @returns {boolean} True if the password meets all conditions, false otherwise.
*/
export function isValidPassword(password: string): boolean {
Copy link
Contributor

@atomrc atomrc Nov 16, 2023

Choose a reason for hiding this comment

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

If you want to use the browser's built in method to validate inputs, you can also use regexp positive lookahead (see https://javascript.info/regexp-lookahead-lookbehind#lookahead).

The regexp would look like this

/(?=.*[a-z])(?=.*[A-Z])(?=.*[1-9])(?=.*[!@#$%^&*()_+\-=[\]{};':"\\|,.<>/?])/

Copy link
Contributor

Choose a reason for hiding this comment

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

updated in 45c45db

currentType: string;
inputPlaceholder: string;
messageHtml: string;
messageText: string;
messageText: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could call this one message and deprecate the messageHtml (since react node is way more suitable for the future)

Copy link
Contributor

@tlebon tlebon Nov 20, 2023

Choose a reason for hiding this comment

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

ive updated this and the other modal comment in 1d4a248 but my sense is that this one is sort of unrelated to the main PR here.

modalUie: string;
onBgClick: () => void;
passwordGenerator?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little too specific. The Modal component should not be aware that there is a password generator that exists. I think this API can be made more generic

Copy link
Contributor

Choose a reason for hiding this comment

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

this one is removed, agreed its unnecessary.

@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch from 1d4a248 to 772e337 Compare November 22, 2023 09:35
@tlebon tlebon force-pushed the feat/SQSERVICES-1975 branch from 9e201f8 to 59d4ab0 Compare December 19, 2023 13:05
@tlebon tlebon merged commit e820de8 into dev Dec 20, 2023
10 checks passed
@tlebon tlebon deleted the feat/SQSERVICES-1975 branch December 20, 2023 09:16
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.

9 participants