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

FE - Temporarily mock metrics CRUD endpoints #38357

Merged
merged 78 commits into from
Feb 6, 2024

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented Feb 1, 2024

Closes #38262

Description

This PR mocks FE endpoint handlers to pretend that any question which has a name starting with "Metric" (case-insensitive) is a metric.
In other words: you can prefix your question names with "Metric" or "metric" to pretend they're metrics and not questions.
Be sure to pass correct type: "metric" to update/create endpoints though.

qnkhuat and others added 30 commits January 24, 2024 11:36
@kamilmielnik kamilmielnik changed the base branch from master to 37366-migrate-dataset-true February 1, 2024 14:41
@kamilmielnik kamilmielnik marked this pull request as ready for review February 1, 2024 14:50
const result = await get(payload);

if (result.name.toLowerCase().startsWith("metric")) {
return { ...result, type: "metric", dataset: false };
Copy link
Member

Choose a reason for hiding this comment

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

dataset: false is still only a temporary measure during the migration period, 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.

Hopefully. We will have both dataset and type, but we should not be using dataset anymore.

Comment on lines 65 to 67
const tweakedPayload = { ...payload, type: "question", dataset: false };
const result = await create(tweakedPayload);
return { ...result, type: "metric" };
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to first create a tweakedPayload with a type "question", only to override it with a type "metric" in the last (return) step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say we want to create a metric.
We will try to call this create endpoint with some payload which will include type: "metric".
Backend won't accept this, as it's not implemented yet.
So this override will actually make FE send type: "question" to BE, but when the result comes back, we still want FE to believe it's a "metric", so we have to update the type.

Copy link
Member

@nemanjaglumac nemanjaglumac left a comment

Choose a reason for hiding this comment

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

Left some questions inline, but the change looks straightforward.
There are still some E2E tests failing, though - probably not related

return { ...result, type: "metric" };
}

return await update(payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

idk we do we use return await without try catch, but seems there shouldn't be any difference comparing with just returning a promise

@kamilmielnik
Copy link
Contributor Author

There are still some E2E tests failing, though - probably not related

Good point. I disabled the overrides in tests so that we can have green CI: 97f2577

Copy link

replay-io bot commented Feb 2, 2024

StatusComplete ↗︎
Commit1ed0536
Results
⚠️ 1 Flaky
2270 Passed

@nemanjaglumac
Copy link
Member

There are still some E2E tests failing, though - probably not related

Good point. I disabled the overrides in tests so that we can have green CI: 97f2577

You might also need to use isCypressActive.

Base automatically changed from 37366-migrate-dataset-true to metric-v2/add-report-card-type-shortcut February 5, 2024 15:18
Base automatically changed from metric-v2/add-report-card-type-shortcut to master February 5, 2024 15:53
@kamilmielnik kamilmielnik changed the base branch from master to metrics-v2 February 6, 2024 06:35
@kamilmielnik kamilmielnik added the no-backport Do not backport this PR to any branch label Feb 6, 2024
@kamilmielnik kamilmielnik merged commit 39895db into metrics-v2 Feb 6, 2024
67 of 68 checks passed
@kamilmielnik kamilmielnik deleted the 38262-mock-metrics-crud branch February 6, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FE - Temporarily mock metrics CRUD endpoints
4 participants