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.

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

Change permissions used for finishing plugin setup #2242

Merged
merged 11 commits into from
Jun 26, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Changed

- Change mobile shift notifications title and subtitle by @imtoori ([#2288](https://github.com/grafana/oncall/pull/2288))
- Change permissions used during setup to better represent actions being taken by @mderynck ([#2242](https://github.com/grafana/oncall/pull/2242))

## Fixed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ describe('PluginConfigPage', () => {
// click the confirm button within the modal, which actually triggers the callback
await userEvent.click(screen.getByText('Remove'));

await screen.findByTestId(successful ? PLUGIN_CONFIGURATION_FORM_DATA_ID : STATUS_MESSAGE_BLOCK_DATA_ID);
// await screen.findByTestId(successful ? PLUGIN_CONFIGURATION_FORM_DATA_ID : STATUS_MESSAGE_BLOCK_DATA_ID);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need some help on fixing this test properly now that the page is reloading on reset.

Copy link
Contributor

@joeyorlando joeyorlando Jun 22, 2023

Choose a reason for hiding this comment

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

hmm, this type of assertion may no longer be possible for the successful scenario. maybe we just replace this assertion by mocking window.location.reload and assert that it was called in this scenario?

We're expecting the configuration form to be present here, but that is reliant on a full page/state reload to arrive at this state. It might be beyond what @testing-library/react and @testing-library/user-event are capable of? Curious to hear what someone from @grafana/grafana-oncall-frontend thinks here?


// assertions
expect(PluginState.checkIfPluginIsConnected).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import React, { FC, useCallback, useEffect, useState } from 'react';

import { Button, Label, Legend, LoadingPlaceholder } from '@grafana/ui';
import { Button, HorizontalGroup, Label, Legend, LoadingPlaceholder } from '@grafana/ui';
import { useLocation } from 'react-router-dom';
import { OnCallPluginConfigPageProps } from 'types';

import logo from 'img/logo.svg';
import PluginState, { PluginStatusResponseBase } from 'state/plugin';
import { GRAFANA_LICENSE_OSS } from 'utils/consts';
import { APP_VERSION, CLOUD_VERSION_REGEX, GRAFANA_LICENSE_CLOUD, GRAFANA_LICENSE_OSS } from 'utils/consts';

import ConfigurationForm from './parts/ConfigurationForm';
import RemoveCurrentConfigurationButton from './parts/RemoveCurrentConfigurationButton';
Expand Down Expand Up @@ -75,13 +74,14 @@ const PluginConfigPage: FC<OnCallPluginConfigPageProps> = ({
const pluginMetaOnCallApiUrl = jsonData?.onCallApiUrl;
const processEnvOnCallApiUrl = process.env.ONCALL_API_URL; // don't destructure this, will break how webpack supplies this
const onCallApiUrl = pluginMetaOnCallApiUrl || processEnvOnCallApiUrl;
const licenseType = pluginIsConnected?.license;
const fallbackLicense = CLOUD_VERSION_REGEX.test(APP_VERSION) ? GRAFANA_LICENSE_CLOUD : GRAFANA_LICENSE_OSS;
mderynck marked this conversation as resolved.
Show resolved Hide resolved
const licenseType = pluginIsConnected?.license || fallbackLicense;

const resetQueryParams = useCallback(() => removePluginConfiguredQueryParams(pluginIsEnabled), [pluginIsEnabled]);

const triggerDataSyncWithOnCall = useCallback(async () => {
await resetMessages();
mderynck marked this conversation as resolved.
Show resolved Hide resolved
setSyncingPlugin(true);
setSyncError(null);

const syncDataResponse = await PluginState.syncDataWithOnCall(onCallApiUrl);

Expand Down Expand Up @@ -144,35 +144,25 @@ const PluginConfigPage: FC<OnCallPluginConfigPageProps> = ({
}
}, [pluginMetaOnCallApiUrl, processEnvOnCallApiUrl, onCallApiUrl, pluginConfiguredRedirect]);

const resetState = useCallback(() => {
const resetMessages = useCallback(() => {
setPluginResetError(null);
setPluginConnectionCheckError(null);
setPluginIsConnected(null);
setSyncError(null);
}, []);

const resetState = useCallback(() => {
resetMessages();
resetQueryParams();
}, [resetQueryParams]);

/**
* NOTE: there is a possible edge case when resetting the plugin, that would lead to an error message being shown
* (which could be fixed by just reloading the page)
* This would happen if the user removes the plugin configuration, leaves the page, then comes back to the plugin
* configuration.
*
* This is because the props being passed into this component wouldn't reflect the actual plugin
* provisioning state. The props would still have onCallApiUrl set in the plugin jsonData, so when we make the API
* call to check the plugin state w/ OnCall API the plugin-proxy would return a 502 Bad Gateway because the actual
* provisioned plugin doesn't know about the onCallApiUrl.
*
* This could be fixed by instead of passing in the plugin provisioning information as props always fetching it
* when this component renders (via a useEffect). We probably don't need to worry about this because it should happen
* very rarely, if ever
*/
const triggerPluginReset = useCallback(async () => {
setResettingPlugin(true);
resetState();

try {
await PluginState.resetPlugin();
window.location.reload();
} catch (e) {
// this should rarely, if ever happen, but we should handle the case nevertheless
setPluginResetError('There was an error resetting your plugin, try again.');
Expand All @@ -196,16 +186,24 @@ const PluginConfigPage: FC<OnCallPluginConfigPageProps> = ({
content = (
<>
<StatusMessageBlock text={pluginConnectionCheckError || pluginResetError} />
<RemoveConfigButton />
<HorizontalGroup>
<Button variant="primary" onClick={triggerDataSyncWithOnCall} size="md">
Retry Sync
</Button>
{licenseType === GRAFANA_LICENSE_OSS ? <RemoveConfigButton /> : null}
</HorizontalGroup>
</>
);
} else if (syncError) {
content = (
<>
<StatusMessageBlock text={syncError} />
<Button variant="primary" onClick={triggerDataSyncWithOnCall} size="md">
Retry Sync
</Button>
<HorizontalGroup>
<Button variant="primary" onClick={triggerDataSyncWithOnCall} size="md">
Retry Sync
</Button>
{licenseType === GRAFANA_LICENSE_OSS ? <RemoveConfigButton /> : null}
mderynck marked this conversation as resolved.
Show resolved Hide resolved
</HorizontalGroup>
</>
);
} else if (!pluginIsConnected) {
Expand All @@ -228,8 +226,8 @@ const PluginConfigPage: FC<OnCallPluginConfigPageProps> = ({
{pluginIsConnected ? (
<>
<p>
Plugin is connected! Continue to Grafana OnCall by clicking the{' '}
<img alt="Grafana OnCall Logo" src={logo} width={18} /> icon over there 👈
Plugin is connected! Continue to Grafana OnCall by clicking OnCall under Alerts & IRM in the navigation over
there 👈
mderynck marked this conversation as resolved.
Show resolved Hide resolved
</p>
<StatusMessageBlock
text={`Connected to OnCall (${pluginIsConnected.version}, ${pluginIsConnected.license})`}
Expand Down
Loading