Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-18775: Remove some deprecated/unsafe lifecycles #4903

Merged
merged 3 commits into from
Mar 11, 2020
Merged

MM-18775: Remove some deprecated/unsafe lifecycles #4903

merged 3 commits into from
Mar 11, 2020

Conversation

jespino
Copy link
Member

@jespino jespino commented Feb 13, 2020

Summary

Remove some deprecated/unsafe lifecycles (mainly related to enterprise related parts of the code)

Ticket Link

MM-18775

@jespino jespino added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 13, 2020
catalintomai
catalintomai reviewed Feb 17, 2020
View reviewed changes
components/admin_console/permission_schemes_settings/permission_schemes_settings.jsx
@@ -62,9 +60,9 @@ export default class PermissionSchemesSettings extends React.PureComponent {
promises.push(this.props.actions.loadSchemeTeams(scheme.id));
}
Promise.all(promises).then(() => this.setState({loading: false, phase2MigrationIsComplete}));
Copy link
Contributor

@catalintomai catalintomai Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

what happens if setState throws? Is it being caught by the outer catch()? According to this: https://twitter.com/dan_abramov/status/770914625206583296 ; "then(a).catch(b) makes errors from a go to b". Are we ok with it here (and if yes, should phase2MigrationIsComplete be true?).

Copy link
Member Author

Choose a reason for hiding this comment

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

This replicates the same behavior than the previous one, I think we can create another ticket here to address the possible swallowing of failures in the Promise.all().then line, probably we should catch and decide what to do there. But I think is not in the scope of this PR because this is about the migration of UNSAFE methods.

@catalintomai What do you think about creating another ticket for better error handling on this components and keep this PR only for the migration from UNSAFE methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, will do.

@jespino jespino removed the 2: Dev Review Requires review by a core commiter label Feb 17, 2020
@jespino jespino requested a review from lindy65 February 26, 2020 10:29
@jespino
Copy link
Member Author

jespino commented Feb 27, 2020

/update-branch

@lindy65 lindy65 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 28, 2020
@lindy65
Copy link
Contributor

lindy65 commented Feb 28, 2020

Thanks @jespino

I did some smoke-testing around permissions and LDAP groups but this will need to be tested more thoroughly by @srkgupta and @DHaussermann post-merge.

Is it possible to add unit / cypress tests for these changes?

@lindy65
Copy link
Contributor

lindy65 commented Mar 10, 2020

Hi @jespino - should a cypress / unit test be added for this?

@jespino
Copy link
Member Author

jespino commented Mar 10, 2020

@lindy65 I think it is a bit out of the scope here. We are not changing any behavior, we are just maintaining the old behavior. We can add some cypress tests, but I would create another ticket for that.

@jespino
Copy link
Member Author

jespino commented Mar 10, 2020

/update-branch

@mattermod
Copy link
Contributor

Mattermost test server updated with git commit 56f9730e5fc592417fd17501c0e493343bd378b7.

Access here: https://mattermost-webapp-pr-4903.test.mattermost.cloud

lindy65
lindy65 approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@lindy65 lindy65 left a comment

Choose a reason for hiding this comment

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

Thanks @jespino - sounds good to me to forego the cypress/unit test then 👍

@lindy65 lindy65 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Mar 10, 2020
@mattermod
Copy link
Contributor

Test server destroyed

@jespino jespino merged commit cdbedc9 into mattermost:master Mar 11, 2020
@jespino jespino added the 4: Reviews Complete All reviewers have approved the pull request label Mar 11, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 11, 2020
sowmiyamuthuraman pushed a commit to sowmiyamuthuraman/mattermost-webapp that referenced this pull request Apr 10, 2020
@jespino @mattermod @sowmiyamuthuraman
MM-18775: Remove some deprecated/unsafe lifecycles (mattermost#4903)
063a548 
Co-authored-by: mattermod <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mkraft mkraft mkraft approved these changes

@lindy65 lindy65 lindy65 approved these changes

@catalintomai catalintomai catalintomai approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
Milestone
No milestone
6 participants
@jespino @lindy65 @mattermod @mkraft @catalintomai @amyblais