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

only first admin sees preparing-workspace screen #9897

Merged
merged 2 commits into from
Feb 25, 2022
Merged

only first admin sees preparing-workspace screen #9897

merged 2 commits into from
Feb 25, 2022

Conversation

neallred
Copy link
Contributor

@neallred neallred commented Feb 25, 2022 •
edited
Loading

Summary

Second admin should not see use case onboarding.

Ticket Link

https://mattermost.atlassian.net/browse/MM-42103 , but only the First admin vs second admin scenario: scenario listed in the QA description.

Related Pull Requests

Not related in the code sense, but there is a separate server PR that addresses the >= 10 posts part of the bug:

Screenshots

first-admin-vs-second-admin.mp4

Release Note

NONE

@neallred neallred added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 25, 2022
@neallred neallred added this to the v6.5.0 milestone Feb 25, 2022
@neallred neallred self-assigned this Feb 25, 2022
@neallred neallred added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 25, 2022
JulienTant
JulienTant suggested changes Feb 25, 2022
View reviewed changes
packages/mattermost-redux/src/selectors/entities/users.ts Outdated
Comment on lines 702 to 704
const userIds = Object.keys(users);
for (const userId of userIds) {
const user = users[userId];
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this could be replaced by a for (const user of users) - do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can almost do that. JS doesn't allow for...of object iteration , but we can iterate on the Object.values which I think is cleaner code and has the perks of being faster: https://jsbench.me/6il02vjfuw/1

Copy link
Member

Choose a reason for hiding this comment

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

image
Slight improvement on my machine, only +~525op/s haha

JulienTant
JulienTant approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@JulienTant JulienTant left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking the time to make a benchmark :)

julmondragon
julmondragon approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@julmondragon julmondragon left a comment

Choose a reason for hiding this comment

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

Looks good!

components/preparing_workspace/preparing_workspace.tsx
@@ -168,6 +170,7 @@ export default function PreparingWorkspace(props: Props) {
const [submitError, setSubmitError] = useState<string | null>(null);
useEffect(() => {
showOnMountTimeout.current = setTimeout(() => setShowFirstPage(true), 40);
props.actions.getProfiles(0, General.PROFILE_CHUNK_SIZE, {roles: General.SYSTEM_ADMIN_ROLE});
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it'll be very unlikely to hit a 101 system admin profiles in a system.
Also, that this service does not support any kind of sorting by creation or similar, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions! I also assume it will be very unlikely to have 101 system admin profiles in any system. The service does support sorting by creation. However, most uses of the selectors don't read the data directly from the server, but from redux, which doesn't retain the sort order since we store it in redux as an object.

@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Feb 25, 2022
@jgilliam17 jgilliam17 added Setup Cloud Test Server Setup a test server using Mattermost Cloud and removed Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Feb 25, 2022
@jgilliam17
Copy link
Contributor

Thanks @neallred
Tested, looks good to merge

  • Verified only first system admin sees the wizard. The onboarding wizard and checklist are not shown to other users.
  • I'm not running E2Es for now as discussed with @neallred, @AshishDhama is working on e2e test fixes that will be handled in a separate PR.

@jgilliam17 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 Feb 25, 2022
@amyblais amyblais added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 25, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 25, 2022
@mm-cloud-bot
Copy link

Test server destroyed

@neallred neallred merged commit aa48d7d into master Feb 25, 2022
@neallred neallred deleted the MM-42103 branch February 25, 2022 21:55
@neallred
Copy link
Contributor Author

/cherry-pick cloud

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost-webapp that referenced this pull request Feb 25, 2022
@neallred @mattermost-build
only first admin sees preparing-workspace screen (mattermost#9897)
41bf799 
* only first admin sees preparing-workspace screen

(cherry picked from commit aa48d7d)
@mattermost-build mattermost-build mentioned this pull request Feb 25, 2022
Merged
@mattermod mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Feb 25, 2022
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Feb 25, 2022
neallred added a commit that referenced this pull request Feb 25, 2022
@mattermost-build @neallred
only first admin sees preparing-workspace screen (#9897) (#9900)
ca03350 
* only first admin sees preparing-workspace screen

(cherry picked from commit aa48d7d)

Co-authored-by: Nathaniel Allred <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JulienTant JulienTant JulienTant approved these changes

@jgilliam17 jgilliam17 jgilliam17 approved these changes

@julmondragon julmondragon julmondragon approved these changes

Assignees

@neallred neallred

Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation release-note-none
Projects
None yet
Milestone
v6.5.0
7 participants
@neallred @jgilliam17 @mm-cloud-bot @mattermod @JulienTant @julmondragon @amyblais