Hide bots and apps UI when managed by Apps Framework #7850

larkox · 2021-04-09T11:43:59Z

Summary

Hide bots and OAuth apps UI when managed by the apps framework.

When the apps framework plugin is disabled, they show as normal apps/bots.

Ticket Link

https://mattermost.atlassian.net/browse/MM-33158

Related Pull Requests

Proxy Plugin: mattermost/mattermost-plugin-apps#138

NONE

catalintomai · 2021-04-09T14:53:58Z

packages/mattermost-redux/src/reducers/entities/integrations.ts

@@ -195,6 +195,66 @@ function oauthApps(state: IDMappedObjects<OAuthApp> = {}, action: GenericAction)
 }
 }

+function appsOAuthAppsIDs(state: string[] = [], action: GenericAction) {


nit: curious why using OAuthAppsIDs (plural) vs BotIDs (singular), since both OAuth app and bot are apps-related artifacts - why not use either singular or plural for both. Is it because we could conceivably have multiple bots per app, but only one OAuth app?

There are two main reasons for this:

Me not being native speaker, and doubting which of the two ways sounded better.

Me not going over the changes and ensure consistency.

That being said, I have no preference over singular or plural, so which one do you think is better? I will make sure it is consistent for the choice we decide.

0/5 on singular vs. plural (prob. singular makes more sense), I just advocate aligning on one vs. another for both.

mickmister

Functionality looks good 👍

I have just a few requests. For my comments related to adding more tests, I'd say to prioritize testing on components that have logic around the props, rather than testing a component that is just passing a prop to a child.

mickmister · 2021-04-09T15:17:11Z

components/integrations/bots/bot.test.tsx

@@ -206,6 +210,7 @@ describe('components/integrations/bots/Bot', () => {
 accessTokens={accessTokens}
 team={team}
 actions={actions}
+ fromApp={false}


Can we add a test for a bot that has fromApp={true}?

mickmister · 2021-04-09T15:19:30Z

components/integrations/bots/bots.test.tsx

@@ -120,6 +126,8 @@ describe('components/integrations/bots/Bots', () => {
 owners={owners}
 users={users}
 actions={actions}
+ appsEnabled={false}
+ appsBotIDs={[]}


Same here, can we have some tests that have app bots involved?

mickmister · 2021-04-09T15:20:37Z

components/integrations/bots/bots.tsx

@@ -169,6 +187,7 @@ export default class Bots extends React.PureComponent<Props, State> {
 accessTokens={(this.props.accessTokens && this.props.accessTokens[bot.user_id]) || {}}
 actions={this.props.actions}
 team={this.props.team}
+ fromApp={this.props.appsBotIDs.includes(bot.user_id)}


Is there an async operation that could momentarily have fromApp={false} before the getAppBotIDs() response comes back?

If getAppBotIDs takes too long, the user will see the whole information, and then disappear when that information finally arrives. We could keep the loading state until we have all the information. What do you think?

I think we should keep showing the loading animation. But if the bot id fetch fails, then show all of the information.

components/integrations/bots/index.ts

mickmister · 2021-04-09T15:23:16Z

components/integrations/installed_oauth_app/installed_oauth_app.test.tsx

@@ -36,6 +36,7 @@ describe('components/integrations/InstalledOAuthApp', () => {
 onRegenerateSecret: jest.fn(),
 onDelete: jest.fn(),
 filter: '',
+ fromApp: false,


Does it make sense to have an apps-related test here?

components/integrations/installed_oauth_apps/index.ts

components/integrations/installed_oauth_apps/installed_oauth_apps.test.tsx

mickmister · 2021-04-09T15:29:43Z

packages/mattermost-redux/src/actions/integrations.ts

+export function getAppsOAuthAppsIDs(): ActionFunc {
+ return bindClientFunc({
+ clientFunc: Client4.getAppsOAuthAppsIDs,
+ onSuccess: [IntegrationTypes.RECEIVED_APPS_OAUTH_APP_IDS],


Do we need the arrays around the onSuccess values? It looks inconsistent throughout the file, but it caught me off guard at first glance here.

No. It is not needed. Nevertheless, for consistency, I would prefer to keep it as a list.

The array is left over from when we always used to have a _SUCCESS action dispatched for every request in addition to something like a POST_RECEIVED action. I think it might still be used in some cases, but we could definitely consider adding a non-array form in the future.

packages/mattermost-redux/src/reducers/entities/integrations.ts

hmhealey

One question, but I assume it's not going to result in any changes unless we want to add some heavy future proofing.

hmhealey · 2021-04-12T18:19:52Z

packages/mattermost-redux/src/client/client4.ts

@@ -2548,6 +2548,20 @@ export default class Client4 {
 );
 };

+ getAppsOAuthAppIDs = () => {


Is there any chance either of these will ever need paging in the future? That's obviously a bigger question than just something that affects the web app, but I find it never hurts to ask

Being small amount of information (a list of ids) I hope there is no case where this may become a problem (e.g. having 100 Apps installed?). That being said, I am open to suggestions or comments.

hmhealey · 2021-04-12T18:20:23Z

packages/mattermost-redux/src/selectors/entities/integrations.ts

+ appsEnabled,
+ (state: GlobalState) => state.entities.integrations.appsOAuthAppIDs,
+ (apps, ids) => {
+ return apps ? ids : [];


DHaussermann · 2021-04-14T21:00:17Z

@larkox there may be an issue here where the UI can revert to the old version. I'm still investigating. I think I turned off bot creation in the system console and re-enabled then the UI no longer appers changed. I'll try and pin this down.

~~For the time being... a question - is @appsbot new to this bersion of the proxy?~~

This should have a Mattermost logo and not Jira. If this bot is unrelated to the changes, I'll make a new issue.

DHaussermann · 2021-04-15T05:03:27Z

We can disregard the comment above. I cannot repro this issue. I took some time to investigate this and it seems as though the bots where left over data from before the corresponding proxy plugin build was deployed. Seem when you remove apps using debug commands, the corresponding bot is left behind which causes some confusion.

DHaussermann

Tested and passed

Backstage UI change to hide config are implemented successfully
Bots and oAuth have the option removed
No changes to conventional bots or oAth apps unrelated to MM Apps
No issues found on subsequent installations when using proper uninstall and reinstall commands
A couple unrelated issues were found. I will create follow up tasks.

LGTM!

DHaussermann · 2021-04-15T20:41:17Z

@larkox I need to test additional cases. The Jira ticket you referenced may require a broader solution than removing the option. We may actually need the ownership to change. 0/5 The Apps plugin should own the bot.

This UI clean-up does not guard against the owner being deactivated. Further more - as pointed out by @mickmister, it causes an additional issue if the sysadmin who owns the bot becomes deactivated (the stock config deactivates the bots owned by this user), as the bot can then no longer be re-enabled.

larkox · 2021-04-16T07:14:27Z

@DHaussermann For the bots, it might be possible to add something like owned by the plugin (actually, we could create these bots with the plugin RPC methods). The potential problem I see is for OAuth Apps, which with the current APIs we would not be able to do something like that.

amyblais · 2021-04-29T22:14:16Z

/check-cla

mattermod · 2021-04-29T22:14:36Z

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

* Hide bots and apps UI when managed by Apps Framework * Make OAuthAppsIDs into OAuthAppIDs * don't hide bot account management controls

amyblais · 2021-04-30T12:13:31Z

@hanzei Can you also help cherry-pick this to cloud?

amyblais · 2021-04-30T12:19:38Z

/cherry-pick cloud

mattermod · 2021-04-30T12:19:40Z

Cherry pick is scheduled.

* Hide bots and apps UI when managed by Apps Framework * Make OAuthAppsIDs into OAuthAppIDs * don't hide bot account management controls (cherry picked from commit 7430755)

Automatic Merge

Hide bots and apps UI when managed by Apps Framework

6def23a

larkox added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Apr 9, 2021

larkox requested review from hmhealey, mickmister and DHaussermann April 9, 2021 11:43

Fix types and i18n

92360f1

larkox mentioned this pull request Apr 9, 2021

Add API for clients to check the bots and oauth apps ids mattermost/mattermost-plugin-apps#138

Merged

Fix snapshot

f119431

catalintomai reviewed Apr 9, 2021

View reviewed changes

mickmister suggested changes Apr 9, 2021

View reviewed changes

larkox added 3 commits April 9, 2021 18:12

Make OAuthAppsIDs into OAuthAppIDs

9e2f4b0

Address feedback

3e0e521

Add tests

2363f6e

larkox requested review from mickmister and catalintomai April 12, 2021 11:17

hmhealey approved these changes Apr 12, 2021

View reviewed changes

mickmister approved these changes Apr 13, 2021

View reviewed changes

Merge branch 'master' into HideAppsBotsUI

459f5a6

DHaussermann approved these changes Apr 15, 2021

View reviewed changes

DHaussermann added QA Review Done Do Not Merge Should not be merged until this label is removed and removed 3: QA Review Requires review by a QA tester QA Review Done labels Apr 15, 2021

DHaussermann added the 3: QA Review Requires review by a QA tester label Apr 15, 2021

hanzei added this to the v5.35.0 milestone Apr 22, 2021

Merge branch 'master' into HideAppsBotsUI

568529c

amyblais assigned larkox Apr 29, 2021

amyblais added the AutoMerge used by Mattermod to merge PR automatically label Apr 29, 2021

hanzei assigned hanzei and unassigned larkox Apr 29, 2021

hanzei added Do Not Merge Should not be merged until this label is removed and removed Do Not Merge Should not be merged until this label is removed AutoMerge used by Mattermod to merge PR automatically labels Apr 29, 2021

cpanato merged commit 7430755 into master Apr 30, 2021

cpanato deleted the HideAppsBotsUI branch April 30, 2021 10:38

hanzei pushed a commit that referenced this pull request Apr 30, 2021

Hide bots and apps UI when managed by Apps Framework (#7850)

622c8f7

* Hide bots and apps UI when managed by Apps Framework * Make OAuthAppsIDs into OAuthAppIDs * don't hide bot account management controls

hanzei 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 Apr 30, 2021

hanzei removed their assignment Apr 30, 2021

amyblais assigned larkox and hanzei Apr 30, 2021

mattermost-build mentioned this pull request Apr 30, 2021

Automated cherry pick of #7850 #8036

Merged

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Apr 30, 2021

mattermod pushed a commit that referenced this pull request Apr 30, 2021

Hide bots and apps UI when managed by Apps Framework (#7850) (#8036)

d1e11fb

Automatic Merge

hanzei unassigned larkox and hanzei Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide bots and apps UI when managed by Apps Framework #7850

Hide bots and apps UI when managed by Apps Framework #7850

larkox commented Apr 9, 2021 •

edited by hanzei

Loading

catalintomai Apr 9, 2021 •

edited

Loading

larkox Apr 9, 2021

catalintomai Apr 9, 2021

mickmister left a comment

mickmister Apr 9, 2021

mickmister Apr 9, 2021

mickmister Apr 9, 2021

larkox Apr 9, 2021

mickmister Apr 13, 2021

mickmister Apr 9, 2021

mickmister Apr 9, 2021

larkox Apr 12, 2021

hmhealey Apr 12, 2021

hmhealey left a comment

hmhealey Apr 12, 2021

larkox Apr 12, 2021

hmhealey Apr 12, 2021

DHaussermann commented Apr 14, 2021 •

edited

Loading

DHaussermann commented Apr 15, 2021

DHaussermann left a comment •

edited

Loading

DHaussermann commented Apr 15, 2021 •

edited

Loading

larkox commented Apr 16, 2021

amyblais commented Apr 29, 2021

mattermod commented Apr 29, 2021

amyblais commented Apr 30, 2021

amyblais commented Apr 30, 2021

mattermod commented Apr 30, 2021

Hide bots and apps UI when managed by Apps Framework #7850

Hide bots and apps UI when managed by Apps Framework #7850

Conversation

larkox commented Apr 9, 2021 • edited by hanzei Loading

Summary

Ticket Link

Related Pull Requests

catalintomai Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister 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

Choose a reason for hiding this comment

hmhealey 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

DHaussermann commented Apr 14, 2021 • edited Loading

DHaussermann commented Apr 15, 2021

DHaussermann left a comment • edited Loading

Choose a reason for hiding this comment

DHaussermann commented Apr 15, 2021 • edited Loading

larkox commented Apr 16, 2021

amyblais commented Apr 29, 2021

mattermod commented Apr 29, 2021

amyblais commented Apr 30, 2021

amyblais commented Apr 30, 2021

mattermod commented Apr 30, 2021

larkox commented Apr 9, 2021 •

edited by hanzei

Loading

catalintomai Apr 9, 2021 •

edited

Loading

DHaussermann commented Apr 14, 2021 •

edited

Loading

DHaussermann left a comment •

edited

Loading

DHaussermann commented Apr 15, 2021 •

edited

Loading