Conversation
What does that mean, can you give me some more info here please? What names are we talking about? |
"Title", "Inhalt", "E-News Nummer", "Datum", "WP Post ID" for example I guess these aren't used in the app but want to be sure 😅 Maybe rather a question to @svenseeberg if these are being used somewhere. |
There was a problem hiding this comment.
This PR doesn't sit right with me. I don't think this is a good approach. I think there are 3 ways to handle this, all with their pros and cons:
- Store News in cache, not database: Have some kind of service fetch news form different sources and store them as JSON in our redis cache. Then have our API simply fetch the cached news from external sources in addition to our internal PushNotifications
- Properly store the external news in our database by transforming them to PushNotifications (with some kind of
sourceflag for clarity) - If the deadline is urgent, copy-paste the code from tunews as a Django app into our repository and dump the content from the existing database. IMO, this only makes sense as an indermediate step, i.e. the code should be removed at some point once we implemented one of the above solutions. this is much easier if we keep it as a separate app.
Introducing a new NewsLanguage model in our cms app feels like a bad idea to me. This is how you get a messy database. No one wants a messy database
| title=post["title"], | ||
| language_code=language.bcp47_tag, | ||
| excerpt=post["content"], | ||
| url=post["link"], |
There was a problem hiding this comment.
We don't have get_absolute_url() for Tü News posts as they are not part of our contents. Is it acceptable to substitute url with link flag for Tü News posts? (examples of Tü News)
|
@jonbulz |
jonbulz
left a comment
There was a problem hiding this comment.
Thanks for working on this topic, this is a solid start! I do have some concerns, though:
Architecture / Extensibility
We already plan to add a third news source (Amal news) in the near future. There may be more to come after that. With this pattern, we have growing if/elif chains in several places. Also, the code for external news lives in push_notifications.py, which is a bit confusing. I think even though we decided against a database model, this deserves its own module and class. I don't want to be too specific with the proposed solution, but if you want, we can discuss this in a meeting.
Resilience
There are several possible unhandled exceptions in the code, see my additional comments. I think we need to be a bit more defensive when dealing with content from external sources. A malformed response should not crash our process.
Other Bugs
There are some other bugs I noticed, e.g. the duplicate URL name, see my comments. Additionally, I think we are still lacking pagination, and the results are not sorted again after tuenews have been added. I'd also prefer if we stick to one naming convention tuenews or tue_news :)
|
@jonbulz
I'm not sure a separate module reduces
|
@steffenkleinle said in this comment:
This endpoint always returns an array of all news items, not a paginated response. Supporting pagination means we have to accept additional request parameters, e.g. Or is pagination not needed after all? |
I guess this depends on the amount of items we are expecting. But would be good practice at least I guess :) |
|
Concerning
What I mean is this: Imagine we want to add another news source. What we'd have to do now is to edit at least 4 files:
Each of those is a place where the source-specific logic is coupled to the general logic. If any of those steps is forgotten, it breaks the new source. or similar. I'd leave the exact implementation up to you, but I think that this abstraction would be very useful. I'm happy to discuss this further if you'd like :) |
Thank you for explanations 😸 I got it what was meant 🙈 I'm but not sure whether the pagination works well with cliend-side filtering. Our Response contains both push nofitication news and Tü News posts. The app filters them by |
I see. Thank you for sharing your idea :) I would like to suggest to build first an agreement in team how the it should be implemented exactly. Otherwise everyone imagines something diffenrent and this PR then results in a table of long discussion 😓 and also want to avoid this PR staying unmegred for a long time as App Team is waiting for us. |
|
@jonbulz @hannaseithe |
31a0b61 to
09e5045
Compare
Well, the TuNews Number is shown in each article: https://integreat.app/tuebingen/de/news/tu-news/2290 But you need to check if we render the number into the article or whether this is already part of the content. I think at some point we had the idea of creating a nice, structured footer with the structured data. But I think its likely that we never did that and never will do ;) |
Yes, its already part of the content, we don't actively use |
hannaseithe
left a comment
There was a problem hiding this comment.
Hello Mizuki, thank you for working this. I do like that you implemented an abstraction layer for the news managers. This makes is much cleaner. In my review I have focussed on improvements on the abstraction and haven't gone much into detail in regards to specific implementations.
|
|
||
| from ...news_managers.abstract_news_manager import AbstractNewsManager | ||
|
|
||
| CHOICES: Final[list[tuple[str, type[AbstractNewsManager]]]] = [ |
There was a problem hiding this comment.
| CHOICES: Final[list[tuple[str, type[AbstractNewsManager]]]] = [ | |
| CHOICES: Final[list[AbstractNewsManager]] = [ |
or
| CHOICES: Final[list[tuple[str, type[AbstractNewsManager]]]] = [ | |
| CHOICES: Final[list[type[AbstractNewsManager]]] = [ |
Meaning:
- I would get rid of the str in the tuple and make that a
nameproperty on the News Manager, - and either already save instances of the Manager in CHOICES (suggestion 1) or make the methods on the News Manager @classmethod s , so that we can follow a singleton(-ish) pattern here.
Reason for 1: The name of the manager should be encapsulated in the manager itself. It seems redundant/unnecessary to keep it as an external lookup key
Reason for 2: Instantiating a new manager every time we want to access a method seems not very clean, even if it works here because we do not have any instance state (yet).
There was a problem hiding this comment.
I go for the suggestion 1 👍
| @abstractmethod | ||
| def collect_news_items( | ||
| self, region_slug: str, language_slug: str, channel: str | ||
| ) -> list[dict]: |
There was a problem hiding this comment.
| ) -> list[dict]: | |
| ) -> list[NewsItem]: |
I would use typing here to validate/guarantee the implicit assumptions about what a NewsItem needs to look like
|
|
||
| from ..cms.models import Language, Region | ||
|
|
||
|
|
There was a problem hiding this comment.
| @dataclass | |
| class NewsItem(): | |
| display_date: Any #we rely upon that existing in news.py |
But if we actually want to guarantee more than that as a data structure in the API (which I assume we do), this should be expanded.
There was a problem hiding this comment.
Hmm 🤔 Do you mean we don't return dict anymore?
|
|
||
| sorted_result = sorted(result, key=lambda i: i["display_date"], reverse=True) | ||
|
|
||
| return JsonResponse(sorted_result, safe=False) |
There was a problem hiding this comment.
I have a problem here with the fact that we are breaking our established pattern of using a transform function inside the endpoint which is our guarantor of the data strcuture that we expose on our API endpoint. Now we have two hidden transform functions that create different data structures and that one has no direct access to on the endpoint module.
I would expect to have a transform_news_item function that is applied to every element in sorted results. So that if I want to look up, which strucutret the endpoint follows, I would just take a look at the transform function (basically they fullfill the a similar function to the serializers in the django rest framework)
There was a problem hiding this comment.
I understand your point, but then Tü News posts are every time transformed when the API is called. Instead they can be directly stored in the form they are delivered. And it's enough consisntent in my opinion if we put the transform functions for news items into respective news managers: the news managers then build an exception that their transform functions are not in the endpoint but they all have their transform function in them. In addition, jumping between a news manager and the endpoint only for this very last step breaks the stream of work.
This is very preference matter. I stick with the current version as long as no large motivation appears for separating transformation from the news managers into the endpoint.
|
One thing I forgot: I feel we should have a test for the new endpoint |
Short description
This PR implements a common endpoint for news (push notifcation) and Tü News.
Proposed changes
all-news/that delivers both PNs and posts of Tü Newssourceflag to distinguish PNs form Tü News postsfcm/,sent_push_notifications) not to break the appAdd Tü News related models and management command to integrate functionalities of our Tü News repositoryThe current approach is based on that we will store Tü News posts in our Integreat CMS database and the memory cache solution proposed here comes laterSide effects
A fieldtranslationsis added which does not exists in digitalfabrik/tunews to provide reference to translations like for PNsGerman verbose names should probably be changed to English 🤔 Is there any harm @svenseeberg @steffenkleinle ?I applied the 28 days rule to nes from Tü News too. Is that okay or unnecessary?Faithfulness to issue description and design
There are no intended deviations from the issue and design.
Currently Tü News Bridge is supporting only German, English, Farsi and Arabic. This PR checkes Tü News posts for all languages that are available in the CMS system.
We probably need a new section in CMS where Service Team can decide for which language we fetch posts from Tü News (not implemented yet). Currently German, English, Farsi and Arabic are handled in our Tü NewsHow to test
import_tuenews❓ Does anyone have an idea how to test the social media header endpoints (
news/<slug:news_type>/<slug:slug>/) ?Self-answer from 24.04.2026: can be tested like this in the console:
Resolved issues
Fixes: #4174
Pull Request Review Guidelines