MM 20547 user settings security ts #6216

sridhar02 · 2020-08-20T07:35:39Z

Summary

Migrate 'components/user_settings/security' module and associated tests to TypeScript

Ticket Link

hanzei · 2020-08-21T11:06:27Z

Thanks for your contribution. Heads up that CI failed with an type check error. You can find the logs here. Let me know if you need help resolving the error.

mattermod · 2020-09-04T01:01:31Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich

nevyangelova

LGTM :)

jasonblais · 2020-09-07T14:16:32Z

Thanks @sridhar02 for the contribution! Let us know if we can with the type check error reported by our CI build. You can find the logs here. We'd be happy to help :)

sridhar02 · 2020-09-07T17:14:47Z

@jasonblais I am busy with some other work, so I was not able to do this, so I will complete it a week or two.If I need I will mention in this PR

hanzei · 2020-09-22T10:24:54Z

@sridhar02 Heads up that CI is failing with https://app.circleci.com/pipelines/github/mattermost/mattermost-webapp/25391/workflows/0c4d5ec4-c3ec-4aeb-bc13-a3c01d3b2916/jobs/165162
Let me know if you have any questions.

sridhar02 · 2020-09-24T03:37:43Z

@hanzei can you help me fixing the type errors in the test file, I am new to testing struggling with it and due to it, I was unable to complete this PR.

hanzei · 2020-09-24T10:02:12Z

components/user_settings/security/user_settings_security.test.tsx

@@ -125,7 +138,7 @@ describe('components/user_settings/display/UserSettingsDisplay', () => {
 preventDefault: jest.fn(),
 };
 wrapper.setState({authorizedApps: apps});
- wrapper.instance().deauthorizeApp(event);
+ (wrapper.instance() as UserSettingsSecurity).deauthorizeApp(event);


You need to update the calls shallow in this file to explicitly set the type to
const wrapper: ShallowWrapper<any, any, UserSettingsSecurity> = shallow(....

With that you can even remove these type cases.

I have written in this const wrapper: any = shallow(<UserSettingsSecurity {...props}/>) using this also I am not getting any errors while checking types will that be okay or I need to change it to const wrapper: ShallowWrapper<any, any, UserSettingsSecurity> ?

I'd prefer you use ShallowWrapper<any, any, UserSettingsSecurity but either way is fine.

hanzei · 2020-09-24T10:02:51Z

@devinbinnie I'm not sure how to fix the issues with deauthorizeApp. Can you help there?

components/user_settings/security/user_settings_security.test.tsx:141:69 - error TS2345: Argument of type '{ currentTarget: { getAttribute: jest.Mock<any, any>; }; preventDefault: jest.Mock<any, any>; }' is not assignable to parameter of type 'MouseEvent<Element, MouseEvent>'.
  Type '{ currentTarget: { getAttribute: Mock<any, any>; }; preventDefault: Mock<any, any>; }' is missing the following properties from type 'MouseEvent<Element, MouseEvent>': altKey, button, buttons, clientX, and 27 more.

141         (wrapper.instance() as UserSettingsSecurity).deauthorizeApp(event);
                                                                        ~~~~~

devinbinnie · 2020-09-24T13:55:08Z

@devinbinnie I'm not sure how to fix the issues with deauthorizeApp. Can you help there?

components/user_settings/security/user_settings_security.test.tsx:141:69 - error TS2345: Argument of type '{ currentTarget: { getAttribute: jest.Mock<any, any>; }; preventDefault: jest.Mock<any, any>; }' is not assignable to parameter of type 'MouseEvent<Element, MouseEvent>'.
  Type '{ currentTarget: { getAttribute: Mock<any, any>; }; preventDefault: Mock<any, any>; }' is missing the following properties from type 'MouseEvent<Element, MouseEvent>': altKey, button, buttons, clientX, and 27 more.

141         (wrapper.instance() as UserSettingsSecurity).deauthorizeApp(event);
                                                                        ~~~~~

@hanzei @sridhar02 You'll want to cast event as an any in this situation, as it's expecting you to provide the entire MouseEvent object. But since we don't want to mock that out, casting as any in the tests is alright.

devinbinnie

Looks good overall, just had a few comments I'd like you to address.
Thanks for the awesome contribution @sridhar02!

devinbinnie · 2020-09-24T17:11:43Z

components/user_settings/security/index.ts


-function mapStateToProps(state, ownProps) {
+type Actions= {


Nit: space between Actions and =

devinbinnie · 2020-09-24T17:50:07Z

components/user_settings/security/user_settings_security.test.tsx

@@ -48,7 +62,7 @@ describe('components/user_settings/display/UserSettingsDisplay', () => {
 enableOAuthServiceProvider: true,
 };

- const wrapper = shallow(<UserSettingsSecurity {...props}/>);
+ const wrapper: any = shallow(<UserSettingsSecurity {...props}/>);


This can actually be of type ShallowWrapper<any, any, UserSettingsSecurity>

devinbinnie · 2020-09-24T17:53:12Z

components/user_settings/security/user_settings_security.test.tsx

@@ -65,48 +79,64 @@ describe('components/user_settings/display/UserSettingsDisplay', () => {
 enableOAuthServiceProvider: true,
 };

- const wrapper = shallow(<UserSettingsSecurity {...props}/>);
+ const wrapper: any = shallow(<UserSettingsSecurity {...props}/>);


Same with this one

devinbinnie · 2020-09-24T17:55:33Z

components/user_settings/security/user_settings_security.tsx

+ currentPassword: string;
+ newPassword: string;
+ confirmPassword: string;
+ passwordError: React.ReactNode | string;


string is part of React.ReactNode so you shouldn't need this as a union type.

devinbinnie

LGTM! Added a couple non-blocking comments but that's all.
Thanks again @sridhar02!

devinbinnie · 2020-09-25T13:34:55Z

components/user_settings/security/user_settings_security.test.tsx

 });

 test('submitPassword() should not have called updateUserPassword', async () => {
 const wrapper = shallow(<UserSettingsSecurity {...requiredProps}/>);

- await wrapper.instance().submitPassword();
+ await (wrapper.instance() as UserSettingsSecurity).submitPassword();


If you use the same type as we added above, you can avoid this cast here.

devinbinnie · 2020-09-25T13:36:00Z

components/user_settings/security/user_settings_security.test.tsx

@@ -125,7 +138,7 @@ describe('components/user_settings/display/UserSettingsDisplay', () => {
 preventDefault: jest.fn(),
 };
 wrapper.setState({authorizedApps: apps});
- wrapper.instance().deauthorizeApp(event);
+ (wrapper.instance() as UserSettingsSecurity).deauthorizeApp(event);


I'd prefer you use ShallowWrapper<any, any, UserSettingsSecurity but either way is fine.

hanzei · 2020-09-25T16:43:37Z

@jgilliam17 A smote test around the security setting should be enough.

jgilliam17

Thank you @sridhar02
Tested, looks good to merge.

Tested User Security Settings, works as expected (password edits, sign in, access history, view/log out of active sessions)

@hanzei Can you please help merge? Thank you 🙂

mm-cloud-bot · 2020-09-25T19:44:57Z

Test server destroyed

…o MM-20457 * 'master' of github.com:Tak-Iwamoto/mattermost-webapp: [MM-28680] fix blank page on undefined message (mattermost#6450) MM-28733 : Admin Advisor v2 (mattermost#6461) MM 20547 user settings security ts (mattermost#6216) MM-20904 Add E2E tests for custom categories (mattermost#5981) Update NOTICE.txt (mattermost#6550) Check if emojiMap is undefined before invoking method for it (mattermost#6495) [MM-28151] Cypress/E2E: Automate backlogs - System Console > User Password (mattermost#6538) [MM-28295] e2e/messaging/reactions: add reactions spec (mattermost#6535) [MM-28706] revert ordering change for `in:` auto-suggest search results (mattermost#6493) Remove usage of t function with useIntl (mattermost#6552) MM-28992 Remove unused notPresent and mustBePresent props from MultiSelectSetting (mattermost#6544) MM-27530 Fix for Multiselect items not scrolling into view on arrow keys (mattermost#6286) Demoting failed test from prod (mattermost#6534) migrate show_more to typescript (mattermost#6501) [MM-28878] Revert style change to remove next steps arrow (mattermost#6527) [MM-28972] Added 'Always On' option for new sidebar (mattermost#6530) MM-28839 Fix for hover of cursor on indicators (mattermost#6524) [MM-28790] allow plugins to open in a different tab (mattermost#6476) MM-28290 Always update team member unread counts when added to a channel (mattermost#6500)

…o MM-20462 * 'master' of github.com:Tak-Iwamoto/mattermost-webapp: fix getType is not a function (mattermost#6566) MM-29028 Remove findDOMNode from Slack import (mattermost#6554) MM-28997 Remove references to old context API (mattermost#6545) Add custom slash command tests (mattermost#6391) Cypress/E2E: Fix guest experience ui spec (mattermost#6563) [MM-28784] Migrate string refs to functional ones (mattermost#6494) [MM-28680] fix blank page on undefined message (mattermost#6450) MM-28733 : Admin Advisor v2 (mattermost#6461) MM 20547 user settings security ts (mattermost#6216) MM-20904 Add E2E tests for custom categories (mattermost#5981) Update NOTICE.txt (mattermost#6550) Check if emojiMap is undefined before invoking method for it (mattermost#6495) [MM-28151] Cypress/E2E: Automate backlogs - System Console > User Password (mattermost#6538) [MM-28295] e2e/messaging/reactions: add reactions spec (mattermost#6535) [MM-28706] revert ordering change for `in:` auto-suggest search results (mattermost#6493) Remove usage of t function with useIntl (mattermost#6552) MM-28992 Remove unused notPresent and mustBePresent props from MultiSelectSetting (mattermost#6544) MM-27530 Fix for Multiselect items not scrolling into view on arrow keys (mattermost#6286) Demoting failed test from prod (mattermost#6534)

* convert files to ts and resolved type errors * resolve type errors * resolved type errors * resolved type errors * type errors resolved * resolved type errors * resolved test type errors * type errors * type errors fix * fix lint error * resolved type errors * Resolved type errors * resolved comments

hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Aug 21, 2020

hanzei requested review from devinbinnie and nevyangelova August 21, 2020 11:04

sridhar02 force-pushed the MM-20547_user-settings-security-ts branch from bb5ebf1 to 84670aa Compare August 24, 2020 05:37

mattermod added the Lifecycle/1:stale label Sep 4, 2020

nevyangelova approved these changes Sep 7, 2020

View reviewed changes

jasonblais removed the Lifecycle/1:stale label Sep 7, 2020

hanzei added the Awaiting Submitter Action Blocked on the author label Sep 15, 2020

hanzei removed the request for review from devinbinnie September 15, 2020 10:32

sridhar02 force-pushed the MM-20547_user-settings-security-ts branch from 84670aa to f024dc1 Compare September 18, 2020 05:56

devinbinnie self-requested a review September 22, 2020 13:25

sridhar02 added 8 commits September 23, 2020 23:26

convert files to ts and resolved type errors

f93e2b5

resolve type errors

9facfcc

resolved type errors

1a0ada8

resolved type errors

d51786b

type errors resolved

e22aa65

resolved type errors

9aa35b8

resolved test type errors

e876020

type errors

4a940c5

sridhar02 force-pushed the MM-20547_user-settings-security-ts branch from f024dc1 to 4a940c5 Compare September 24, 2020 01:57

sridhar02 added 2 commits September 24, 2020 08:09

type errors fix

7700bbf

fix lint error

ab23293

hanzei reviewed Sep 24, 2020

View reviewed changes

sridhar02 added 2 commits September 24, 2020 22:02

resolved type errors

65ae211

Resolved type errors

d232aca

sridhar02 requested a review from hanzei September 24, 2020 17:07

devinbinnie removed the Awaiting Submitter Action Blocked on the author label Sep 24, 2020

devinbinnie suggested changes Sep 24, 2020

View reviewed changes

resolved comments

a4e2cab

sridhar02 requested a review from devinbinnie September 25, 2020 01:50

devinbinnie approved these changes Sep 25, 2020

View reviewed changes

hanzei removed their request for review September 25, 2020 16:42

hanzei removed the 2: Dev Review Requires review by a core commiter label Sep 25, 2020

hanzei requested a review from jgilliam17 September 25, 2020 16:43

jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Sep 25, 2020

jgilliam17 approved these changes Sep 25, 2020

View reviewed changes

jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Sep 25, 2020

devinbinnie merged commit e5d2ebf into mattermost:master Sep 25, 2020

hanzei added this to the v5.30.0 milestone Sep 25, 2020

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM 20547 user settings security ts #6216

MM 20547 user settings security ts #6216

sridhar02 commented Aug 20, 2020

hanzei commented Aug 21, 2020

mattermod commented Sep 4, 2020

nevyangelova left a comment

jasonblais commented Sep 7, 2020

sridhar02 commented Sep 7, 2020

hanzei commented Sep 22, 2020 •

edited

Loading

sridhar02 commented Sep 24, 2020 •

edited

Loading

hanzei Sep 24, 2020

sridhar02 Sep 24, 2020 •

edited

Loading

devinbinnie Sep 25, 2020

hanzei commented Sep 24, 2020

devinbinnie commented Sep 24, 2020

devinbinnie left a comment

devinbinnie Sep 24, 2020

sridhar02 Sep 25, 2020

devinbinnie Sep 24, 2020

sridhar02 Sep 25, 2020

devinbinnie Sep 24, 2020

sridhar02 Sep 25, 2020

devinbinnie Sep 24, 2020

sridhar02 Sep 25, 2020

devinbinnie left a comment

devinbinnie Sep 25, 2020

devinbinnie Sep 25, 2020

hanzei commented Sep 25, 2020

jgilliam17 left a comment •

edited by hanzei

Loading

mm-cloud-bot commented Sep 25, 2020

MM 20547 user settings security ts #6216

MM 20547 user settings security ts #6216

Conversation

sridhar02 commented Aug 20, 2020

Summary

Ticket Link

hanzei commented Aug 21, 2020

mattermod commented Sep 4, 2020

nevyangelova left a comment

Choose a reason for hiding this comment

jasonblais commented Sep 7, 2020

sridhar02 commented Sep 7, 2020

hanzei commented Sep 22, 2020 • edited Loading

sridhar02 commented Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

sridhar02 Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei commented Sep 24, 2020

devinbinnie commented Sep 24, 2020

devinbinnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinbinnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei commented Sep 25, 2020

jgilliam17 left a comment • edited by hanzei Loading

Choose a reason for hiding this comment

mm-cloud-bot commented Sep 25, 2020

hanzei commented Sep 22, 2020 •

edited

Loading

sridhar02 commented Sep 24, 2020 •

edited

Loading

sridhar02 Sep 24, 2020 •

edited

Loading

jgilliam17 left a comment •

edited by hanzei

Loading