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

Local news list sorting #2952

Closed
PeterNerlich opened this issue Oct 8, 2024 · 13 comments
Closed

Local news list sorting #2952

PeterNerlich opened this issue Oct 8, 2024 · 13 comments
Labels
discussion-needed good first issue Good for newcomers Native Affects the native project shared Affects the shared project Task Web Affects the web project

Comments

@PeterNerlich
Copy link

PeterNerlich commented Oct 8, 2024

Is your feature request related to a problem? Please describe.

Sometimes local news (push notifications) that are older than 28 days appear in the message list / seemingly out of order. This is confusing.

Describe the solution you'd like

  • Sort the news by max(last_updated, timestamp) timestamp
  • Set timestamp to max(last_updated, timestamp) timestamp` (TBD)
  • Filter out news where lastUpdate > 28 days

Describe alternatives you've considered

Leave it as is

Additional context

CMS issue: digitalfabrik/integreat-cms#2994

This was discussed in the Integreat CMS team, we concluded that there is nothing in our API that would influence this and it has to be implemented on app side.

@ztefanie ztefanie added the reproduction-needed Cannot be reproduced label Oct 9, 2024
@ztefanie
Copy link
Member

ztefanie commented Oct 9, 2024

@PeterNerlich Thanks for this issue.
What do you mean by message list? This one: https://integreat.app/testumgebung/de/news/local

@PeterNerlich
Copy link
Author

@ztefanie Yes, that one

@steffenkleinle steffenkleinle added Web Affects the web project Native Affects the native project labels Oct 9, 2024
@steffenkleinle
Copy link
Member

steffenkleinle commented Oct 25, 2024

@PeterNerlich do you know whether we should filter out news that are older than 28 days? Or should filtering still be done exclusively in the CMS? If we filter in the apps, we'd have another place we'd have to adjust this time period if we ever decide to change it. I also remember some discussions about that.

@nikolahoff Furthermore, our last update timestamp should also reflect the same logic (i.e. lastUpdate = max(sent, last_modified)), correct? Otherwise just sorting differently could still confuse if the sorting behavior is not comprehensable.

@steffenkleinle steffenkleinle added good first issue Good for newcomers discussion-needed shared Affects the shared project and removed reproduction-needed Cannot be reproduced labels Oct 25, 2024
@PeterNerlich
Copy link
Author

@PeterNerlich do you know whether we should filter out news that are older than 28 days?

I have not worked on this part of the API and don't know past discussions around that, but as we do already limit which results we put out, so there is no need to do that on app side. This issue is only about sorting, e.g. when news are edited and should thus appear as recent again.

@steffenkleinle
Copy link
Member

@PeterNerlich do you know whether we should filter out news that are older than 28 days?

I have not worked on this part of the API and don't know past discussions around that, but as we do already limit which results we put out, so there is no need to do that on app side. This issue is only about sorting, e.g. when news are edited and should thus appear as recent again.

Thank you @PeterNerlich for the explanation.

@steffenkleinle steffenkleinle moved this to Next Up in team-app Oct 28, 2024
@steffenkleinle steffenkleinle moved this from Next Up to In Progress in team-app Oct 30, 2024
@steffenkleinle steffenkleinle changed the title Push notification list sorting Local news list sorting Oct 30, 2024
@steffenkleinle
Copy link
Member

@PeterNerlich I just had a more detailed look on the API resonse and it seems like there are only two relevant properties timestamp and last_update. However, last_update is the same as timestamp, just not formatted as ISO with timezones.
We do neither get a sent nor a last_modified and therefore can't adjust the sorting.
Am I missing something here? See https://cms-test.integreat-app.de/api/v3/testumgebung/de/fcm/?channel=news

One another note, it seems like at least updates are correctly reflected in the timestamp property and the sorting is also reflected accordingly. In what case this would break? With scheduled sending?

@PeterNerlich
Copy link
Author

PeterNerlich commented Oct 31, 2024

@steffenkleinle Whoops, we messed up, sorry. I have the code defining this endpoint in front of me and see that we do already apply some sorting when delivering the messages.

Do you already do some sorting app-side as well? If not, I propose that we just edit the existing sorting in the CMS and you don't have to do anything for this after all, as well as setting the last_updated property to the higher one between its original value and sent_date.

@steffenkleinle
Copy link
Member

steffenkleinle commented Oct 31, 2024

@steffenkleinle Whoops, we messed up, sorry. I have the code defining this endpoint in front of me and see that we do already apply some sorting when delivering the messages.

Do you already do some sorting app-side as well? If not, I propose that we just edit the existing sorting in the CMS and you don't have to do anything for this after all, as well as setting the last_updated property to the higher one between its original value and sent_date.

@PeterNerlich We don't have any app-side sorting yet, no. I think that would be good if the logic then stays on your side, yes. So I can close the issue, right?
Thank you 💚

Just to make sure, the following is something you would do? Not something we should?

as well as setting the last_updated property to the higher one between its original value and sent_date.

@PeterNerlich
Copy link
Author

Yes, I will change last_updated to be at least sent_date. Or would it make more sense to you to expose both sent_date and last_updated as they are saved in the database and introduce a third value (or let you take the max of both on app side)?

@steffenkleinle
Copy link
Member

Yes, I will change last_updated to be at least sent_date. Or would it make more sense to you to expose both sent_date and last_updated as they are saved in the database and introduce a third value (or let you take the max of both on app side)?

No, I think its best if you just set timestamp to max(sent_date, last_updated). We don't use last_updated anymore as it does not include timezones.

@steffenkleinle
Copy link
Member

This will be done on the CMS side.

@steffenkleinle steffenkleinle closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in team-app Oct 31, 2024
@PeterNerlich
Copy link
Author

No, I think its best if you just set timestamp to max(sent_date, last_updated). We don't use last_updated anymore as it does not include timezones.

@steffenkleinle It seems that timestamp was the value provided by the API originally: It has the comment "deprecated field in the future"

Also, it does seem to include timezones to me, but different from timestamp it is not UTC anymore (…Z) but in the set localtime of the server (?) and specifying the offset of that timezone (…+02:00)

I will ask the team how to proceed

@steffenkleinle
Copy link
Member

No, I think its best if you just set timestamp to max(sent_date, last_updated). We don't use last_updated anymore as it does not include timezones.

@steffenkleinle It seems that timestamp was the value provided by the API originally: It has the comment "deprecated field in the future"

@PeterNerlich you mean last_modified? That was the original value and is not used anymore. timestamp, however, is still very much used and should stay.

Also, it does seem to include timezones to me, but different from timestamp it is not UTC anymore (…Z) but in the set localtime of the server (?) and specifying the offset of that timezone (…+02:00)

I will ask the team how to proceed

Exactly. Thank you. Basically our requirement would be that timestamp continues to work and should be set to max of sent and last updated dates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-needed good first issue Good for newcomers Native Affects the native project shared Affects the shared project Task Web Affects the web project
Projects
Archived in project
Development

No branches or pull requests

4 participants