Skip to content

PB-1635 Hide expired items in search #572

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

asteiner-swisstopo
Copy link
Contributor

This hides expired items in the /search endpoints. That this wasn't done already only became apparent when a bug prevented the cron job cron-delete-expired to clean up expired items properly, see PB-1575.

There are two search endpoints affected by this change:

  1. GET /search: Simple filtering
  2. POST /search: Advanced filtering in the payload.

We hide an item if has property "expires" set to a time in the past, so in principle the same as in these endpoints:

To discuss:

  1. There is a small difference to the Item endpoints. I first check if the field is defined and then compare timestamps, not vice versa:

    • Items endpoints: Q(properties_expires__gte=timezone.now()) | Q(properties_expires=None)
    • Search endpoints: Q(properties_expires=None) | Q(properties_expires__gte=timezone.now())

    I don't see why you would check for None only after... Is this a bug?

  2. To check whether an expiry date is in the past, the reference time is the time at which the query is processed on our server. As storage and filtering is done on the same server, I don't see problems with time zones.

@adk-swisstopo
Copy link
Member

I don't see why you would check for None only after... Is this a bug?

Why would the order matter?
That said, maybe there should be a common get_visible_items method that both endpoints call?

@adk-swisstopo
Copy link
Member

While I think this looks good, I generally think it's nicer to guard this kind of behaviour change behind a flag we can control from an environment variable. This allows to roll back easily in case it causes a problem. See FEATURE_AUTH_ENABLE_APIGW for an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants