maykinmedia / open-archiefbeheer

0 stars 0 forks source link

Reviewing processed review may lead to error #441

Closed svenvandescheur closed 1 month ago

svenvandescheur commented 1 month ago

NOTE: THIS MAY BE INTENTIONAL: https://github.com/maykinmedia/open-archiefbeheer/issues/372

Currently: it seems that reviewing a destruction list with a processed review seems to show all zaken, where only the review items are expected. After fixing this (by using the in_review filter on the zaken endpoint) a new problem occurs:

After submitting the following error is returned:

    "zakenReviews": [
        "You can only provide feedback about cases that are part of the destruction list."
    ]
}

This may happen probably because in some cases the status of a destruction list item can be set to "removed" instead of "suggested", I've looked into into why this happens and I thinkg it's either due to too strict check:

backend/src/openarchiefbeheer/destruction/api/serializers.py:414

destruction_list_items = (
    attrs["destruction_list"]
    .items.filter(status=ListItemStatus.suggested)
    .select_related("zaak")
    .values_list("zaak__url", flat=True)
)

or due to the filter (in_review) showing too many zaken:

backend/src/openarchiefbeheer/zaken/api/filtersets.py:223

def filter_in_review(
    self, queryset: QuerySet[Zaak], name: str, value: Decimal
) -> QuerySet[Zaak]:
    review = DestructionListReview.objects.prefetch_related(
        "item_reviews__destruction_list_item__zaak"
    ).get(pk=value)
    zaken_urls = review.item_reviews.all().values_list(
        "destruction_list_item__zaak__url", flat=True
    )
    return queryset.filter(url__in=zaken_urls)

Altering filter_in_review to:

SUGGESTION

  def filter_in_review(
      self, queryset: QuerySet[Zaak], name: str, value: Decimal
  ) -> QuerySet[Zaak]:
      review = DestructionListReview.objects.prefetch_related(
          "item_reviews__destruction_list_item__zaak"
      ).get(pk=value)
      zaken_urls = review.item_reviews.exclude(
          destruction_list_item__status=ListItemStatus.removed
      ).values_list("destruction_list_item__zaak__url", flat=True)
      return queryset.filter(url__in=zaken_urls)

hides "removed" results from the destruction list.

However, I'm confused about the meaning of DestructionListItemAction, does "keep" mean keep (don't destroy) the zaak or does it mean "keep" the review suggesting it to be removed. In any case: we should check the frontend API payload to validate if the correct behaviour is implemented and check if it works correctly.