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

feat: api 404 handling #870

Merged
merged 14 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions frontend/e2e/amending-law-overview.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,53 @@ test("should display a loading error message when the API call fails", async ({

await page.unrouteAll()
})

test("should redirect to the 404 page when the amending law JSON does not exist", async ({
page,
}) => {
await page.route(
"**/norms/eli/bund/bgbl-1/2023/413/2023-12-29/1/deu/regelungstext-1?",
async (route, request) => {
if (request.headers()["accept"] === "application/json") {
await route.fulfill({
status: 404,
})
} else {
await route.continue()
}
},
)

await page.goto(
"/amending-laws/eli/bund/bgbl-1/2023/413/2023-12-29/1/deu/regelungstext-1",
)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})

test("should redirect to the 404 page when the amending law HTML does not exist", async ({
page,
}) => {
await page.route(
"**/norms/eli/bund/bgbl-1/2023/413/2023-12-29/1/deu/regelungstext-1?",
async (route, request) => {
if (request.headers()["accept"] === "text/html") {
await route.fulfill({
status: 404,
})
} else {
await route.continue()
}
},
)

await page.goto(
"/amending-laws/eli/bund/bgbl-1/2023/413/2023-12-29/1/deu/regelungstext-1",
)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})
28 changes: 28 additions & 0 deletions frontend/e2e/amending-law-references.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,31 @@ test("should be able to select a mod, add two new ref's and delete one using the
await expect(ref1Highlight).toBeHidden()
await expect(ref2Highlight).toBeVisible()
})

test.describe("Amending Law References Page Error Handling", () => {
test("Redirect to 404 if XML not found", async ({ page }) => {
await page.route(
"**/norms/eli/bund/bgbl-1/1002/10/1002-01-10/1/deu/regelungstext-1?",
async (route, request) => {
if (
request.method() === "GET" &&
request.headers()["accept"] === "application/xml"
) {
await route.fulfill({
status: 404,
})
} else {
await route.continue()
}
},
)

await page.goto(
"/amending-laws/eli/bund/bgbl-1/1002/10/1002-01-10/1/deu/regelungstext-1/affected-documents/eli/bund/bgbl-1/1002/1/1002-01-01/1/deu/regelungstext-1/references",
)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})
})
26 changes: 26 additions & 0 deletions frontend/e2e/amending-laws-article-editor-common.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,4 +240,30 @@ test.describe("XML and HTML tabs", () => {
page.getByText("Artikel 1Änderung des Vereinsgesetzes"),
).toBeVisible()
})

test("Handle 404 error for XML in tabs", async ({ page }) => {
await page.route(
"**/norms/eli/bund/bgbl-1/2017/s419/2017-03-15/1/deu/regelungstext-1?",
async (route, request) => {
if (
request.method() === "GET" &&
request.headers()["accept"] === "application/xml"
) {
await route.fulfill({
status: 404,
})
} else {
await route.continue()
}
},
)

await page.goto(
"/amending-laws/eli/bund/bgbl-1/2017/s419/2017-03-15/1/deu/regelungstext-1/articles/hauptteil-1_art-1/edit",
)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -222,4 +222,42 @@ test.describe("Error handling for Temporal Data page", () => {
}),
).toBeVisible()
})

test("redirects to 404 page when entry into force HTML is not found", async ({
page,
}) => {
await page.route(
"/api/v1/norms/eli/bund/bgbl-1/2017/s419/2017-03-15/1/deu/regelungstext-1/articles?refersTo=geltungszeitregel",
(route) => {
route.fulfill({
status: 404,
})
},
)

await page.goto(BASE_URL)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})

test("redirects to 404 page when temporal data time boundaries are not found", async ({
page,
}) => {
await page.route(
"/api/v1/norms/eli/bund/bgbl-1/2017/s419/2017-03-15/1/deu/regelungstext-1/timeBoundaries",
(route) => {
route.fulfill({
status: 404,
})
},
)

await page.goto(BASE_URL)

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,19 @@ test.describe("Redirect and start page content", () => {
}),
).toBeVisible()
})

test("should display a no data message when the API returns an empty array", async ({
page,
}) => {
await page.route("**/api/v1/announcements", (route) => {
route.fulfill({
status: 200,
body: JSON.stringify([]),
})
})

await page.goto("/")

await expect(page.getByText("Keine Verkündungen gefunden.")).toBeVisible()
})
})
15 changes: 0 additions & 15 deletions frontend/e2e/render-not-found-page.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,4 @@ test.describe("404 Page", () => {
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})

test(`should display 404 page when API response is 404`, async ({ page }) => {
await page.route("/api/v1/announcements", (route) => {
route.fulfill({
status: 404,
body: "Not Found",
})
})

await page.goto("/amending-laws")

await expect(
page.getByRole("heading", { name: /404 - Seite nicht gefunden/ }),
).toBeVisible()
})
})
23 changes: 2 additions & 21 deletions frontend/src/services/apiService.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,6 @@ describe("useApiFetch", () => {
)
})

it("redirects to the 404 page if the API returns a 404", async () => {
vi.spyOn(window, "fetch").mockResolvedValue(
new Response("{}", { status: 404 }),
)

const mockPush = vi.fn().mockResolvedValue({})
vi.doMock("@/router", () => ({ default: { push: mockPush } }))

const { useApiFetch } = await import("@/services/apiService")

useApiFetch("foo/bar")

await vi.waitFor(() =>
expect(mockPush).toHaveBeenCalledWith({ name: "NotFound" }),
)

vi.doUnmock("@/router")
})

it("aborts the request if the URL is marked as invalid", async () => {
const fetchSpy = vi
.spyOn(window, "fetch")
Expand Down Expand Up @@ -115,7 +96,7 @@ describe("useApiFetch", () => {
const { error } = useApiFetch("/example", { immediate: true }).json()

await vi.waitFor(() => {
expect(error.value).toEqual({ type: "/errors/example" })
expect(error.value).toEqual({ type: "/errors/example", status: 404 })
})
})

Expand All @@ -129,7 +110,7 @@ describe("useApiFetch", () => {
const { error } = useApiFetch("/example", { immediate: true }).json()

await vi.waitFor(() => {
expect(error.value).toEqual({ type: "__fallback__" })
expect(error.value).toEqual({ type: "__fallback__", status: 404 })
})
})
})
17 changes: 6 additions & 11 deletions frontend/src/services/apiService.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { getFallbackError } from "@/lib/errorResponseMapper"
import routerInstance from "@/router"
import { createFetch, UseFetchReturn } from "@vueuse/core"

/**
Expand Down Expand Up @@ -59,15 +58,6 @@ export const useApiFetch = createFetch({
},

onFetchError(fetchContext) {
// We'll probably remove this again because we don't want a blanket
// redirect to 404 if anything goes wrong.
if (fetchContext.response?.status === 404 && routerInstance) {
routerInstance.push({ name: "NotFound" }).catch((err) => {
if (err.name !== "NavigationDuplicated") {
console.error("Failed to navigate to 404 page:", err)
}
})
}
// this error is sometimes throws when previous requests are automatically aborted as
// some of the data changed and refetch is true. It seems to only be throws when the request
// is aborted before it was actually send.
Expand All @@ -93,7 +83,12 @@ export const useApiFetch = createFetch({
// Since we're only ever interested in the data and never the error object,
// we'll replace the `fetchContext.error` with the response data so we can
// access it in the UI.
fetchContext.error = fetchContext.data ?? getFallbackError()
const baseError = fetchContext.data ?? getFallbackError()

fetchContext.error = {
...baseError,
status: fetchContext.response?.status ?? null,
}

return fetchContext
},
Expand Down
18 changes: 14 additions & 4 deletions frontend/src/views/amending-law/AmendingLaw.view.vue
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import RisNavbarSide, {
import { useEliPathParameter } from "@/composables/useEliPathParameter"
import { getFrbrDisplayText } from "@/lib/frbr"
import { useGetNorm } from "@/services/normService"
import { ref } from "vue"
import { RouterView } from "vue-router"
import { ref, watch } from "vue"
import RisErrorCallout from "@/components/controls/RisErrorCallout.vue"
import { RouterView, useRouter } from "vue-router"

const router = useRouter()

const menuItems: LevelOneMenuItem[] = [
{
Expand Down Expand Up @@ -40,6 +42,14 @@ const menuItems: LevelOneMenuItem[] = [

const eli = useEliPathParameter()
const { data: amendingLaw, isFetching, error } = useGetNorm(eli)
watch(
() => error.value,
(err) => {
if (err && err.status === 404) {
router.push({ name: "NotFound" })
}
},
)

const breadcrumbs = ref<HeaderBreadcrumb[]>([
{
Expand All @@ -61,8 +71,8 @@ const breadcrumbs = ref<HeaderBreadcrumb[]>([
<RisLoadingSpinner />
</div>

<div v-else-if="error || !amendingLaw" class="m-24">
<RisErrorCallout :error />
<div v-else-if="error" class="m-24">
<RisErrorCallout v-if="error.status !== 404" :error />
</div>

<div
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { useEliPathParameter } from "@/composables/useEliPathParameter"
import { useNormXml } from "@/composables/useNormXml"
import { getFrbrDisplayText } from "@/lib/frbr"
import { useGetNorm } from "@/services/normService"
import { computed, ref } from "vue"
import { computed, ref, watch } from "vue"
import { useRoute, useRouter } from "vue-router"

/* -------------------------------------------------- *
Expand Down Expand Up @@ -90,6 +90,15 @@ const breadcrumbs = ref<HeaderBreadcrumb[]>([
},
{ key: "textMetadataEditor", title: "Textbasierte Metadaten" },
])

watch(
() => amendingNormXmlError?.value,
(err) => {
if (err?.status === 404) {
router.push({ name: "NotFound" })
}
},
)
</script>

<template>
Expand All @@ -106,7 +115,11 @@ const breadcrumbs = ref<HeaderBreadcrumb[]>([
</div>

<div
v-else-if="amendingNormError || affectedNormError || amendingNormXmlError"
v-else-if="
amendingNormError ||
affectedNormError ||
(amendingNormXmlError && amendingNormXmlError.status !== 404)
"
class="p-24"
>
<RisErrorCallout
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ import { LawElementIdentifier } from "@/types/lawElementIdentifier"
import { useDebounce } from "@vueuse/core"
import { computed, Ref, ref, toValue, watch } from "vue"
import RisErrorCallout from "@/components/controls/RisErrorCallout.vue"
import { useRouter } from "vue-router"

const router = useRouter()
const eid = useEidPathParameter()
const eli = useEliPathParameter()
const {
Expand Down Expand Up @@ -129,6 +131,12 @@ function handlePreviewKeyDown(e: KeyboardEvent) {
selectAllMods()
}
}

watch(loadXmlError, (err) => {
if (err?.status === 404) {
router.push({ name: "NotFound" })
}
})
</script>

<template>
Expand Down Expand Up @@ -167,7 +175,10 @@ function handlePreviewKeyDown(e: KeyboardEvent) {
</div>

<div v-else-if="loadXmlError">
<RisErrorCallout :error="loadXmlError" />
<RisErrorCallout
v-if="loadXmlError.status !== 404"
:error="loadXmlError"
/>
</div>

<RisTabs
Expand Down
Loading
Loading