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

[#378] Prevent crash if reviewItem has zaak = null #379

Merged
merged 4 commits into from
Sep 26, 2024
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
5 changes: 3 additions & 2 deletions backend/src/openarchiefbeheer/destruction/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,9 @@ def create(self, validated_data: dict) -> DestructionListReview:
class DestructionListItemReviewSerializer(serializers.ModelSerializer):
zaak = serializers.SerializerMethodField(
help_text=_(
"In the case that the zaak has already been deleted, only the URL field will be returned."
)
"In the case that the zaak has already been deleted, this field will be null."
),
allow_null=True,
)

class Meta:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,38 @@ def create_data():
await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000")

await self.then.zaaktype_filters_are(page, ["ZAAKTYPE-01 (ZAAKTYPE-01)"])

@tag("gh-378")
async def test_zaak_removed_outside_process(self):
@sync_to_async
def create_data():
record_manager = UserFactory.create(username="Record Manager", password="ANic3Password", role__can_start_destruction=True)

zaken = ZaakFactory.create_batch(2)
list = DestructionListFactory.create(
author=record_manager,
assignee=record_manager,
status=ListStatus.changes_requested,
uuid="00000000-0000-0000-0000-000000000000",
name="Destruction list to process",
)
item1 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[0])
item2 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[1])

review = DestructionListReviewFactory.create(destruction_list=list, decision=ReviewDecisionChoices.rejected)
DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item1, review=review)
DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item2, review=review)

# Simulate the zaak being deleted by *something else*
item1.zaak.delete()

async with browser_page() as page:
await self.given.data_exists(create_data)
await self.when.record_manager_logs_in(page)
await self.then.path_should_be(page, "/destruction-lists")

await self.when.user_clicks_button(page, "Destruction list to process")

await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000")
await self.then.page_should_contain_text(page, "Opnieuw indienen")
await self.then.this_number_of_zaken_should_be_visible(page, 1)
Original file line number Diff line number Diff line change
Expand Up @@ -270,3 +270,39 @@ def create_data():
await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000/review")

await self.then.zaaktype_filters_are(page, ["ZAAKTYPE-01 (ZAAKTYPE-01)", "ZAAKTYPE-02 (ZAAKTYPE-02)"])

@tag("gh-378")
async def test_zaak_removed_outside_process(self):
@sync_to_async
def create_data():
record_manager = UserFactory.create(role__can_start_destruction=True)
reviewer = UserFactory.create(username="Beoordelaar", password="ANic3Password", role__can_review_destruction=True)

zaken = ZaakFactory.create_batch(2)
list = DestructionListFactory.create(
author=record_manager,
assignee=reviewer,
status=ListStatus.ready_to_review,
uuid="00000000-0000-0000-0000-000000000000",
name="Destruction list to review",
)
item1 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[0])
item2 = DestructionListItemFactory.create(destruction_list=list, zaak=zaken[1])

review = DestructionListReviewFactory.create(destruction_list=list, decision=ReviewDecisionChoices.rejected)
DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item1, review=review)
DestructionListItemReviewFactory.create(destruction_list=list, destruction_list_item=item2, review=review)

# Simulate the zaak being deleted by *something else*
item1.zaak.delete()

async with browser_page() as page:
await self.given.data_exists(create_data)
await self.when.reviewer_logs_in(page)
await self.then.path_should_be(page, "/destruction-lists")

await self.when.user_clicks_button(page, "Destruction list to review")

await self.then.path_should_be(page, "/destruction-lists/00000000-0000-0000-0000-000000000000/review")
await self.then.page_should_contain_text(page, "Accorderen")
await self.then.this_number_of_zaken_should_be_visible(page, 1)
1 change: 1 addition & 0 deletions backend/src/openarchiefbeheer/zaken/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

@admin.register(Zaak)
class ZaakAdmin(admin.ModelAdmin):
search_fields = ("identificatie", "uuid")

def get_urls(self):
urls = super().get_urls()
Expand Down
12 changes: 7 additions & 5 deletions frontend/src/fixtures/reviewItem.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { ReviewItem } from "../lib/api/review";
import { ReviewItemWithZaak } from "../lib/api/review";
import { createArrayFactory, createObjectFactory } from "./factory";
import { zaakFactory, zakenFactory } from "./zaak";

const FIXTURE_REVIEW_ITEM: ReviewItem = {
const FIXTURE_REVIEW_ITEM: ReviewItemWithZaak = {
pk: 1,
zaak: zaakFactory(),
feedback: "Deze niet",
};

const FIXTURE_REVIEW_ITEMS: ReviewItem[] = [
const FIXTURE_REVIEW_ITEMS: ReviewItemWithZaak[] = [
FIXTURE_REVIEW_ITEM,
{
pk: 2,
Expand All @@ -22,7 +22,9 @@ const FIXTURE_REVIEW_ITEMS: ReviewItem[] = [
},
];

const reviewItemFactory = createObjectFactory<ReviewItem>(FIXTURE_REVIEW_ITEM);
const reviewItemsFactory = createArrayFactory<ReviewItem>(FIXTURE_REVIEW_ITEMS);
const reviewItemFactory =
createObjectFactory<ReviewItemWithZaak>(FIXTURE_REVIEW_ITEM);
const reviewItemsFactory =
createArrayFactory<ReviewItemWithZaak>(FIXTURE_REVIEW_ITEMS);

export { reviewItemFactory, reviewItemsFactory };
6 changes: 6 additions & 0 deletions frontend/src/lib/api/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ export type ZaakReview = {
};

export type ReviewItem = {
pk: number;
zaak: Zaak | null;
feedback: string;
};

export type ReviewItemWithZaak = {
pk: number;
zaak: Zaak;
feedback: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
import { listSelectielijstKlasseChoices } from "../../../lib/api/private";
import {
Review,
ReviewItem,
ReviewItemWithZaak,
getLatestReview,
listReviewItems,
} from "../../../lib/api/review";
Expand Down Expand Up @@ -46,7 +46,7 @@ export interface DestructionListDetailContext {
user: User;

review: Review | null;
reviewItems: ReviewItem[] | null;
reviewItems: ReviewItemWithZaak[] | null;

selectieLijstKlasseChoicesMap: Record<string, Option[]> | null;
}
Expand Down Expand Up @@ -83,15 +83,21 @@ export const destructionListDetailLoader = loginRequired(
})
: null;

// #378 - If for some unfortunate reason a zaak has been deleted outside of the process,
// item.zaak can be null
const reviewItemsWithZaak = reviewItems
? (reviewItems.filter((item) => !!item.zaak) as ReviewItemWithZaak[])
: reviewItems;

/**
* Fetch selectable zaken: empty array if review collected OR all zaken not in another destruction list.
* FIXME: Accept no/implement real pagination?
*/
const getDestructionListItems =
async (): Promise<PaginatedDestructionListItems> =>
reviewItems
reviewItemsWithZaak
? {
count: reviewItems.length,
count: reviewItemsWithZaak.length,
next: null,
previous: null,
results: [],
Expand Down Expand Up @@ -122,13 +128,13 @@ export const destructionListDetailLoader = loginRequired(
* reviewItems ? await listSelectieLijstKlasseChoices({}) : null,
*/
const getReviewItems = () =>
reviewItems
reviewItemsWithZaak
? cacheMemo(
"selectieLijstKlasseChoicesMap",
async () =>
Object.fromEntries(
await Promise.all(
reviewItems.map(async (ri) => {
reviewItemsWithZaak.map(async (ri) => {
const choices = await listSelectielijstKlasseChoices({
zaak: ri.zaak.url,
});
Expand All @@ -137,12 +143,12 @@ export const destructionListDetailLoader = loginRequired(
),
),
// @ts-expect-error - Params not used in function but in case key only.
reviewItems.map((ri) => ri.pk),
reviewItemsWithZaak.map((ri) => ri.pk),
)
: null;

const getSelectableZaken = () =>
reviewItems || destructionList.status === "ready_to_delete"
reviewItemsWithZaak || destructionList.status === "ready_to_delete"
? ({
count: 0,
next: null,
Expand Down Expand Up @@ -196,7 +202,7 @@ export const destructionListDetailLoader = loginRequired(
user,

review: review,
reviewItems: reviewItems,
reviewItems: reviewItemsWithZaak,

selectieLijstKlasseChoicesMap,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from "../../../lib/api/destructionLists";
import {
Review,
ReviewItem,
ReviewItemWithZaak,
getLatestReview,
listReviewItems,
} from "../../../lib/api/review";
Expand Down Expand Up @@ -37,7 +37,7 @@ export type DestructionListReviewContext = {

paginatedZaken: PaginatedZaken;
review: Review;
reviewItems?: ReviewItem[];
reviewItems?: ReviewItemWithZaak[];
reviewResponse?: ReviewResponse;
reviewers: User[];

Expand Down Expand Up @@ -94,8 +94,15 @@ export const destructionListReviewLoader = loginRequired(
storageKey,
);

const zakenOnPage = reviewItems?.length
? reviewItems.map((ri) => ri.zaak.url as string)
// #378 - If for some unfortunate reason a zaak has been deleted outside of the process,
// item.zaak can be null
// TODO refactor: This code is the same as for the DestructionListDetail loader.
const reviewItemsWithZaak = reviewItems
? (reviewItems.filter((item) => !!item.zaak) as ReviewItemWithZaak[])
: reviewItems;

const zakenOnPage = reviewItemsWithZaak?.length
? reviewItemsWithZaak.map((ri) => ri.zaak.url as string)
: zaken.results.map((z) => z.url as string);

const approvedZaakUrlsOnPagePromise = await Promise.all(
Expand Down Expand Up @@ -123,7 +130,7 @@ export const destructionListReviewLoader = loginRequired(

paginatedZaken: zaken,
review: latestReview,
reviewItems,
reviewItems: reviewItemsWithZaak,
reviewResponse,
reviewers,

Expand Down
Loading