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

[GH-2857] Allow only macOS to call setSecureKeyboardEntryEnabled #2860

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

devinbinnie
Copy link
Member

Summary

It would appear that the app.setSecureKeyboardEntryEnabled function is undefined for Windows and Linux, instead of escaped for those OSes since it doesn't affect them.

This PR ensures that we only call this function for macOS.

Ticket Link

Closes #2857

NONE

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Sep 28, 2023
@devinbinnie devinbinnie added this to the v5.5.0 milestone Sep 28, 2023
Copy link
Member

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @devinbinnie!

Comment on lines +152 to +155
if (process.platform !== 'darwin') {
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

Any tests we can add for this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

None that would add any value.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have the username/password inputs covered in E2E tests? That would be a good one to have to catch any of these type of issues in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes many E2E tests require a login so those should be catching this.

@marianunez
Copy link
Member

@yasserfaraazkhan heads up that we are including this fix for #2857 in the upcoming dot release so we can test post-merge in the RC build.

@marianunez marianunez added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Sep 28, 2023
@marianunez marianunez merged commit 2a88175 into mattermost:master Sep 28, 2023
11 checks passed
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/desktop that referenced this pull request Sep 28, 2023
@mattermost-build mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Sep 28, 2023
marianunez pushed a commit that referenced this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Crash]: _electron.app.setSecureKeyboardEntryEnabled is not a function
5 participants