Sort news/push notifications

[PeterNerlich]

Short description

News items are sent out as push notifications to subscribed devices as well as served via our API. Push notifications are saved as separate objects in our database in order to retain the information for which news items we already sent notifications and to keep that separate from the concept of the content of the news item itself.

News items have a property last_updated, which describes when the content was last edited. Push Notification objects have a sent_date, which describes when the notification for this news item in that channel and this language was sent out. In the API, we only expose last_updated, which is the value also being displayed in the app. This can cause confusion whenever the date the news item was last edited is far before the date when it was actually sent.

Proposed changes

• Add a value display_date to each news item returned by the API which equals either last_updated or sent_date of the corresponding push notification, whichever is greater (effectively clamping the value of last_updated to be no earlier than sent_date)
• Sort the news entries returned by the API by the display_date value instead of last_updated

Alternatively, we could decide to alter last_updated or timestamp to the new behaviour. It seems that we deprecated timestamp in 2022 in favour of the new last_updated, but it is still in use by the app. There are hints that converting the timestamp to be in the local time zone of the server is the reason, but we should ask directly if we wish to pursue such an alternative.

Side effects

• After this PR is merged and released, the app will need to switch from last_updated timestamp over to display_date. No further change should be required.
  If it aligns with how the app team handles this, Local news list sorting integreat-app#2952 might be repurposed for that request.

Resolved issues

Fixes: #2994

---

Pull Request Review Guidelines

[codeclimate]

Code Climate has analyzed commit afd2f3a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.4% (0.0% change).

View more on Code Climate.

[MizukiTemma]

Looks good 😸

[MizukiTemma]

After this PR is merged and released, the app will need to switch from last_updated timestamp over to display_date. No further change should be required.
If it aligns with how the app team handles this, digitalfabrik/integreat-app#2952 might be repurposed for that request.

@steffenkleinle FYI

[steffenkleinle]

After this PR is merged and released, the app will need to switch from last_updated timestamp over to display_date. No further change should be required.
If it aligns with how the app team handles this, digitalfabrik/integreat-app#2952 might be repurposed for that request.

@steffenkleinle FYI

Thanks for the ping. Could we perhaps adjust the naming to last_update to be consistent with all other endpoints? In the end, sending a push notification is also some sort of update to the news item. Otherwise, this looks good as is.

I assume that the API will stay backwards compatible and keeps delivering the timestamp property?

I think the information that there is a last_updated property for news as well was just missed by us. Therefore it would be cool if we could just use that now (including the new functionality/logic).

[david-venhoff]

Not a review, but I noticed that this change should be documented in our api documentation

[JoeyStk]

Thank you very much!
Could you please still add the documentation as @david-venhoff had pointed out and rename the endpoint as @steffenkleinle had pointed out? That'd be great :)
Other than that I don't have any further requests :)

[JoeyStk]

@PeterNerlich what is the state of this PR? :)

[PeterNerlich]

@steffenkleinle Sorry for the long delay. After procrastinating over adjusting this as you suggested, or adding display_date to every endpoint for consistency, or merging it as is for way too long because I didn't really feel comfortable with either option, I asked for the opinion of the team. They saw it fair to just merge it with display_date – in the future, we will hopefully implement a proper strategy for versioning our API, and on the next major version last_update will probably behave like display_date does now. I hope this is an acceptable solution!

[JoeyStk]

LGTM - apart from the failing checks 🚀

[steffenkleinle]

@steffenkleinle Sorry for the long delay. After procrastinating over adjusting this as you suggested, or adding display_date to every endpoint for consistency, or merging it as is for way too long because I didn't really feel comfortable with either option, I asked for the opinion of the team. They saw it fair to just merge it with display_date – in the future, we will hopefully implement a proper strategy for versioning our API, and on the next major version last_update will probably behave like display_date does now. I hope this is an acceptable solution!

Sorry, but I am a little lost here. What does that mean for us? Can you provide a short summary which properties exist now with which behavior and which one we should use for sorting? Or is the sorting done entirely in the CMS already?

[PeterNerlich]

@steffenkleinle Sure thing!

• last_update is kept as is for now
• display_date is the new field with the intended behaviour of assuming either the date the notification was sent, or when the content was last updated, whichever is newer. This is the value that should be shown in the app from now on
• The CMS is delivering this list pre-sorted, newest display_date first. I'm not sure if you want to rely on that or make sure we don't send you bogus data though, you are the representation layer after all
• Other endpoints are not affected