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.

Sign up for GitHub

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

Jump to bottom

Workspace switcher #1052

Merged
merged 55 commits into from
Sep 8, 2021
Merged

Workspace switcher #1052

merged 55 commits into from
Sep 8, 2021

Conversation

harshilsharma63
Copy link
Member

@harshilsharma63 harshilsharma63 commented Aug 20, 2021 •
edited
Loading

Summary

Workspace switcher. Only for Focalboard running as a plugin. Intentionally disabled on a personal server.

Ticket Link

https://community-daily.mattermost.com/plugins/focalboard/workspace/qgsck6cts3fwpqwyjiupjm5cde/47aa9bb4-6967-4a96-83c7-11bd6b20f1eb/392c8889-6c8f-4cb7-8407-f5c57c1acfe8?c=36f12ec9-0705-4a85-84d1-ea1e7040f2f1

Functionality

  1. No change in functionality for personal server mode.
  2. New workspace switcher is now visible when running Focalboard as a Mattermost plugin.
  3. The workspace switcher shows the Focalboard workspaces (named by their corresponding Mattermost channels" and the number of boards in each workspace, for the workspaces the user has access to.
  4. If a workspace has no boards, it's not shown in the list.
  5. Selecting a workspace from the dropdown takes the user to that workspace.

Testing note - please do test on both PostgreSQL and MySQL, and if possible, on MySQL 5.8 as well (current min supported MySQL version).

@harshilsharma63 harshilsharma63 requested a review from a team as a code owner August 20, 2021 09:59
sbishel
sbishel approved these changes Aug 20, 2021
View reviewed changes
Copy link
Collaborator

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Just minor nits.

server/services/store/sqlstore/workspaces.go Outdated Show resolved Hide resolved
server/services/store/sqlstore/workspaces.go Outdated Show resolved Hide resolved
webapp/src/components/workspaceSwitcher/workspaceOptions.tsx Outdated Show resolved Hide resolved
webapp/src/pages/boardPage.tsx Outdated Show resolved Hide resolved
jespino
jespino reviewed Aug 23, 2021
View reviewed changes
webapp/src/octoClient.ts Outdated Show resolved Hide resolved
jespino
jespino reviewed Aug 23, 2021
View reviewed changes
webapp/src/store/users.ts Outdated Show resolved Hide resolved
harshilsharma63
harshilsharma63 commented Sep 3, 2021
View reviewed changes
server/services/store/sqlstore/migrations/migrations_files/000011_match_collation.up.sql Outdated
SET @targetCollation = (SELECT table_collation from information_schema.tables WHERE table_name = 'Channels') AND table_schema = (SELECT DATABASE());

-- blocks
SET @updateCollationQuery = CONCAT('ALTER TABLE focalboard_blocks COLLATE ', @targetCollation);
Copy link
Member Author

Choose a reason for hiding this comment

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

We need all this to use a variable in the query.

jespino
jespino reviewed Sep 6, 2021
View reviewed changes
config.json Outdated Show resolved Hide resolved
jespino
jespino requested changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Some small changes needed, but otherwise looks good to me.

@harshilsharma63 harshilsharma63 added the 2: QA Review Requires review by a QA tester label Sep 6, 2021
jespino
jespino approved these changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Now LGTM. An small nit, but I don't think is strictly necessary.

@harshilsharma63
Copy link
Member Author

@ogi-m this PR is not dev-approved. Can you please test this out?

@ogi-m
Copy link
Contributor

ogi-m commented Sep 7, 2021

Thanks @harshilsharma63, LGTM 🎉

  • workspace switcher visible when running Focalboard as a Mattermost plugin.
  • workspace switcher shows the Focalboard workspaces named by their corresponding Mattermost channels and the number of boards in each workspace
  • selecting a workspace from the dropdown takes the user to that workspace.
  • only the workspaces the user has access to are shown
  • adding/removing a user from a channel adds/removes the channel's workspace to/from the user's workspace switcher
  • If a workspace has no boards, it's not shown in the list
  • workspaces are added to the switcher when boards are added to the the workspace, and removed if all boards get deleted
  • tested on Postgres and MySQL

Note: didn't test on MySQL 5.8, I don't think we have a test server for it but I'll check.

@ogi-m ogi-m removed the 2: QA Review Requires review by a QA tester label Sep 7, 2021
@harshilsharma63 harshilsharma63 merged commit 08db4fe into main Sep 8, 2021
@harshilsharma63 harshilsharma63 deleted the workspace-switcher branch September 8, 2021 04:52
kamre
kamre reviewed Sep 10, 2021
View reviewed changes
webapp/src/components/sidebar/sidebar.scss
.sidebarSwitcher {
display: none;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@harshilsharma63 why is this needed? This is a kind of regression for personal server because now it is not possible to hide the side bar on wide screens.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kamre That's a UX decision the team took - to only allow hiding the sidebar when the window size is small.

DO you think it's something that should be supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in my opinion there are situations when the space is not enough even on wide screens.

Look for example at these kanban boards from default templates, they all have horizontal scroll:
BoardsScroll
This is on laptop with 150% scale in Windows settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @asaadmahmood

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @harshilsharma63, sent PR #1219 to you for review to only hide the collapse-sidebar button for workspaces.

harshilsharma63 added a commit that referenced this pull request Sep 15, 2021
@harshilsharma63 @asaadmahmood
Workspace switcher (#1052)
32985d0 
* API WIP

* WIP

* Finished changes

* Fixed colors:

* Don't enforce charset adn collation in migration, pick from database DSN

* Added MySQL query

* Updated mocks

* Added tests

* Lint fixes

* Fixed typo and removed unsed style

* Checked in a snapshot

* Updated snapshot

* Updated Cypress test

* Updated Cypress test

* Updated Cypress test

* Fixed review comments

* Fixed tests

* Added default collation for MySQL

* Added documentation for ensuring correct database collation

* Updated migrations

* Fixed a bug with collation

* Fixed lint errors

* Used correct collation

* debugging

* Updating css

* Minor UI changes

* USe inbuilt default collation

* Used only charset for mysql

* Fixed linter issue:

* Added migration for matching collation

* Reverted local config changes

* Reverted local config changes

* Handled the case of personal server running on MySQL

* WIP

* Now running collation matching migration onlyt for plugins

* Minor optimization

* Multiple review fixes

* Added group by clause to primary query

* Supported for subpacth

Co-authored-by: Asaad Mahmood <asaadmahmood@users.noreply.github.com>
@kamre kamre mentioned this pull request Sep 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenilim chenilim chenilim left review comments

@kamre kamre kamre left review comments

@mgdelacroix mgdelacroix mgdelacroix left review comments

@sbishel sbishel sbishel approved these changes

@asaadmahmood asaadmahmood asaadmahmood approved these changes

@jespino jespino jespino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@harshilsharma63 @ogi-m @mgdelacroix @jespino @kamre @asaadmahmood @sbishel @chenilim