From ad38a0186f7d278377412961e3288ea4098af613 Mon Sep 17 00:00:00 2001 From: Frank Niessink Date: Wed, 26 Feb 2025 17:23:48 +0100 Subject: [PATCH] When adding a new source to a metric, don't show the spinner until the source has been configured sufficiently to start collecting data. Fixes #9994. --- .../frontend/src/__fixtures__/fixtures.js | 7 ++-- .../src/measurement/MeasurementValue.jsx | 8 ++++- .../src/measurement/MeasurementValue.test.jsx | 26 +++++++++++++- .../frontend/src/metric/MetricDetails.jsx | 12 +++++-- .../src/metric/MetricDetails.test.jsx | 19 ++++++++--- components/frontend/src/utils.jsx | 27 ++++++++++++++- components/frontend/src/utils.test.jsx | 34 +++++++++++++++++-- docs/src/changelog.md | 1 + 8 files changed, 120 insertions(+), 14 deletions(-) diff --git a/components/frontend/src/__fixtures__/fixtures.js b/components/frontend/src/__fixtures__/fixtures.js index df01fd9c9d..cd51a2e59d 100644 --- a/components/frontend/src/__fixtures__/fixtures.js +++ b/components/frontend/src/__fixtures__/fixtures.js @@ -28,6 +28,9 @@ export const dataModel = { metrics: { metric_type: { name: "Metric type", tags: [] }, }, + sources: { + source_type: { name: "Source type", metrics: ["metric_type"], parameters: {} }, + }, } export const report = { @@ -42,7 +45,7 @@ export const report = { type: "metric_type", tags: ["other tag"], target: "1", - sources: { source_uuid: { name: "Source" } }, + sources: { source_uuid: { name: "Source", type: "source_type" } }, status: "target_not_met", recent_measurements: [], latest_measurement: { count: 1 }, @@ -54,7 +57,7 @@ export const report = { tags: ["tag"], target: "2", issue_ids: ["ABC-42"], - sources: { source_uuid2: { name: "Source 2" } }, + sources: { source_uuid2: { name: "Source 2", type: "source_type" } }, status: "informative", recent_measurements: [], latest_measurement: { count: 2 }, diff --git a/components/frontend/src/measurement/MeasurementValue.jsx b/components/frontend/src/measurement/MeasurementValue.jsx index 73348294db..90aafa2149 100644 --- a/components/frontend/src/measurement/MeasurementValue.jsx +++ b/components/frontend/src/measurement/MeasurementValue.jsx @@ -15,6 +15,7 @@ import { isMeasurementOutdated, isMeasurementRequested, isMeasurementStale, + isSourceConfigurationComplete, sum, } from "../utils" import { IgnoreIcon, LoadingIcon } from "../widgets/icons" @@ -104,6 +105,7 @@ export function MeasurementValue({ metric, reportDate }) { value = formatMetricValue(scale, value) const unit = getMetricUnit(metric, dataModel) const stale = isMeasurementStale(metric, reportDate) + const complete = isSourceConfigurationComplete(dataModel, metric) const outdated = isMeasurementOutdated(metric) const requested = isMeasurementRequested(metric) const hasIgnoredEntities = sum(ignoredEntitiesCount(metric.latest_measurement)) > 0 @@ -115,6 +117,10 @@ export function MeasurementValue({ metric, reportDate }) { This may indicate a problem with Quality-time itself. Please contact a system administrator. + + The source configuration of this metric is not complete. Add at least one source and make sure + all mandatory parameters for all sources have been provided. + The source configuration of this metric was changed after the latest measurement. @@ -143,7 +149,7 @@ export function MeasurementValue({ metric, reportDate }) { } > - {measurementValueLabel(hasIgnoredEntities, stale, outdated || requested, value)} + {measurementValueLabel(hasIgnoredEntities, stale, complete && (outdated || requested), value)} ) } diff --git a/components/frontend/src/measurement/MeasurementValue.test.jsx b/components/frontend/src/measurement/MeasurementValue.test.jsx index 9d8f62abd2..69e9fa1652 100644 --- a/components/frontend/src/measurement/MeasurementValue.test.jsx +++ b/components/frontend/src/measurement/MeasurementValue.test.jsx @@ -13,14 +13,21 @@ function renderMeasurementValue({ status = null, type = "violations", unit = null, + url = "https://example.org", } = {}) { return render( - + { it("renders an unkown value", async () => { const { container } = renderMeasurementValue({ latest_measurement: { count: { value: null } } }) expect(screen.getAllByText(/\?/).length).toBe(1) + expect(screen.queryAllByTestId("LoopIcon").length).toBe(0) await expectNoAccessibilityViolations(container) }) it("renders a value that has not been measured yet", async () => { const { container } = renderMeasurementValue() expect(screen.getAllByText(/\?/).length).toBe(1) + expect(screen.queryAllByTestId("LoopIcon").length).toBe(0) + await expectNoAccessibilityViolations(container) +}) + +it("renders a value that can not be measured yet", async () => { + const { container } = renderMeasurementValue({ + latest_measurement: { + count: { value: 1 }, + outdated: true, + start: new Date().toISOString(), + end: new Date().toISOString(), + }, + url: "", + }) + expect(screen.getAllByText(/1/).length).toBe(1) + expect(screen.queryAllByTestId("LoopIcon").length).toBe(0) await expectNoAccessibilityViolations(container) }) diff --git a/components/frontend/src/metric/MetricDetails.jsx b/components/frontend/src/metric/MetricDetails.jsx index 978a2dc199..6048a1af97 100644 --- a/components/frontend/src/metric/MetricDetails.jsx +++ b/components/frontend/src/metric/MetricDetails.jsx @@ -23,7 +23,7 @@ import { import { Logo } from "../source/Logo" import { SourceEntities } from "../source/SourceEntities" import { Sources } from "../source/Sources" -import { getSourceName, isMeasurementRequested } from "../utils" +import { getSourceName, isMeasurementRequested, isSourceConfigurationComplete } from "../utils" import { ButtonRow } from "../widgets/ButtonRow" import { ActionButton } from "../widgets/buttons/ActionButton" import { DeleteButton } from "../widgets/buttons/DeleteButton" @@ -37,16 +37,22 @@ import { MetricDebtParameters } from "./MetricDebtParameters" import { TrendGraph } from "./TrendGraph" function RequestMeasurementButton({ metric, metric_uuid, reload }) { + const dataModel = useContext(DataModel) + const configurationComplete = isSourceConfigurationComplete(dataModel, metric) const measurementRequested = isMeasurementRequested(metric) return ( } itemType="metric" loading={measurementRequested} onClick={() => set_metric_attribute(metric_uuid, "measurement_requested", new Date().toISOString(), reload)} - popup={`Measure this metric as soon as possible`} + popup={ + configurationComplete + ? "Measure this metric as soon as possible" + : "The source configuration of this metric is not complete. Add at least one source and make sure all mandatory parameters for all sources have been provided." + } /> ) } diff --git a/components/frontend/src/metric/MetricDetails.test.jsx b/components/frontend/src/metric/MetricDetails.test.jsx index deeb6d684d..20f595e250 100644 --- a/components/frontend/src/metric/MetricDetails.test.jsx +++ b/components/frontend/src/metric/MetricDetails.test.jsx @@ -76,7 +76,11 @@ const dataModel = { subjects: { subject_type: { metrics: ["violations"] } }, } -async function renderMetricDetails(stopFilteringAndSorting, connection_error, failLoadingMeasurements) { +async function renderMetricDetails({ + stopFilteringAndSorting = null, + connection_error = null, + failLoadingMeasurements = false, +} = {}) { measurement_api.get_metric_measurements.mockImplementation(() => { if (failLoadingMeasurements) { return Promise.reject(new Error("failed to load measurements")) @@ -152,7 +156,7 @@ it("removes the existing hashtag from the URL to share", async () => { }) it("displays whether sources have errors", async () => { - const { container } = await renderMetricDetails(null, "Connection error") + const { container } = await renderMetricDetails({ connection_error: "Connection error" }) expect(screen.getByText(/Sources/)).toHaveClass("error") await expectNoAccessibilityViolations(container) }) @@ -165,7 +169,7 @@ it("displays whether sources have warnings", async () => { it("moves the metric", async () => { const mockCallback = vi.fn() - await renderMetricDetails(mockCallback) + await renderMetricDetails({ stopFilteringAndSorting: mockCallback }) await act(async () => fireEvent.click(screen.getByRole("button", { name: /Move metric to the last row/ }))) expect(mockCallback).toHaveBeenCalled() expect(measurement_api.get_metric_measurements).toHaveBeenCalled() @@ -189,9 +193,16 @@ it("measures the metric", async () => { ) }) +it("does not measure the metric if the metric source configuration is not complete", async () => { + dataModel.sources["sonarqube"].parameters = { url: { mandatory: true, metrics: ["violations"] } } + await renderMetricDetails() + fireEvent.click(screen.getByText(/Measure metric/)) + expect(fetch_server_api.fetch_server_api).not.toHaveBeenCalled() +}) + it("fails to load measurements", async () => { history.push("?expanded=metric_uuid:5") - const { container } = await renderMetricDetails(null, null, true) + const { container } = await renderMetricDetails({ failLoadingMeasurements: true }) expect(screen.queryAllByText(/Loading measurements failed/).length).toBe(1) await expectNoAccessibilityViolations(container) }) diff --git a/components/frontend/src/utils.jsx b/components/frontend/src/utils.jsx index 9ea3aeb2dc..d6cd76592a 100644 --- a/components/frontend/src/utils.jsx +++ b/components/frontend/src/utils.jsx @@ -4,6 +4,7 @@ import { PERMISSIONS } from "./context/Permissions" import { defaultDesiredResponseTimes } from "./defaults" import { STATUSES_NOT_REQUIRING_ACTION } from "./metric/status" import { + dataModelPropType, datePropType, metricPropType, metricsPropType, @@ -104,12 +105,36 @@ export function isMeasurementOutdated(metric) { if (metric.latest_measurement) { return metric.latest_measurement.outdated ?? false } - return Object.keys(metric.sources ?? {}).length > 0 // If there are sources, measurement is needed + return false } isMeasurementOutdated.propTypes = { metric: metricPropType, } +export function isSourceConfigurationComplete(dataModel, metric) { + // Return whether the metric can be measured, meaning that there is at least one source and all sources have + // all mandatory parameters configured + const sources = Object.values(metric.sources ?? {}) + if (sources.length === 0) { + return false + } + return sources.every((source) => { + const parameters = dataModel.sources[source.type].parameters + return Object.entries(parameters).every(([parameterKey, parameter]) => { + if (parameter.mandatory && parameter.metrics.includes(metric.type) && !parameter.default_value) { + // If the parameter is mandatory, has no default value, and is applicable to the metric, + // check if it is configured: + return source?.parameters?.[parameterKey] + } + return true + }) + }) +} +isSourceConfigurationComplete.propTypes = { + dataModel: dataModelPropType, + metric: metricPropType, +} + export function isMeasurementRequested(metric) { if (metric.measurement_requested) { if (metric.latest_measurement) { diff --git a/components/frontend/src/utils.test.jsx b/components/frontend/src/utils.test.jsx index 58d2af5132..680a87be59 100644 --- a/components/frontend/src/utils.test.jsx +++ b/components/frontend/src/utils.test.jsx @@ -17,6 +17,7 @@ import { isMeasurementOutdated, isMeasurementRequested, isMeasurementStale, + isSourceConfigurationComplete, niceNumber, nrMetricsInReport, nrMetricsInReports, @@ -509,14 +510,43 @@ it("returns whether a metric's measurement is outdated", () => { expect(isMeasurementOutdated({})).toBe(false) // A metric without measurements with an empty collection of sources is not outdated: expect(isMeasurementOutdated({ sources: {} })).toBe(false) - // A metric without measurements but with a source is outdated: - expect(isMeasurementOutdated({ sources: { source_uuid: {} } })).toBe(true) + // A metric without measurements but with a source is not outdated: + expect(isMeasurementOutdated({ sources: { source_uuid: {} } })).toBe(false) // A metric with an up-to-date measurement is not outdated: expect(isMeasurementOutdated({ latest_measurement: {} })).toBe(false) // A metric with an outdated measurement is outdated: expect(isMeasurementOutdated({ latest_measurement: { outdated: true } })).toBe(true) }) +it("returns whether a metric can be measured", () => { + const dataModel = { + sources: { source_type: { parameters: { url: { mandatory: true, metrics: ["metric_type"] } } } }, + } + const metric = { type: "metric_type" } + // A metric without sources is not measurable: + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(false) + // A metric with a source without parameters is not measurable: + metric.sources = { source_uuid: { type: "source_type" } } + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(false) + // A metric with a source with empty parameters is measurable: + metric.sources["source_uuid"].parameters = {} + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(false) + // A metric with a source with mandatory parameters configured is measurable: + metric.sources["source_uuid"].parameters["url"] = "https://example.org" + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(true) + // Non-mandatory parameters that are not configured don't make the metric unmeasurable: + dataModel.sources["source_type"].parameters["username"] = { mandatory: false, metrics: ["metric_type"] } + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(true) + // Mandatory parameters for other metrics that are not configured don't make the metric unmeasurable: + metric.sources["source_uuid"].parameters["password"] = { mandatory: true, metrics: ["other_metric_type"] } + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(true) + // A metric with a source with mandatory parameters that has a default value for a mandatory parameter is + // measurable: + dataModel.sources["source_type"].parameters["url"]["default_value"] = "https://example.org" + metric.sources["source_uuid"].parameters = {} + expect(isSourceConfigurationComplete(dataModel, metric)).toBe(true) +}) + it("returns the reference documentation URL", () => { const url = `${DOCUMENTATION_URL}/reference.html` // Simple name diff --git a/docs/src/changelog.md b/docs/src/changelog.md index ba84ce9ca4..1fc960c1bc 100644 --- a/docs/src/changelog.md +++ b/docs/src/changelog.md @@ -21,6 +21,7 @@ If your currently installed *Quality-time* version is not the latest version, pl ### Fixed +- When adding a new source to a metric, don't show the spinner until the source has been configured sufficiently to start collecting data. Fixes [#9994](https://github.com/ICTU/quality-time/issues/9994). - Increase contrast for disabled items in the menu bar. Fixes [#10840](https://github.com/ICTU/quality-time/issues/10840). - Links to documentation on Read the Docs for subjects, metrics, or sources with hyphens in their name wouldn't scroll to the right location. Fixes [#10843](https://github.com/ICTU/quality-time/issues/10843). - Metric details were not shown in exports to PDF. Fixes [#10845](https://github.com/ICTU/quality-time/issues/10845).