Conversation
frcroth
force-pushed
the
feed
branch
2 times, most recently
from
8efdf01 to
8e0080a
Compare
| from myhpi.feed.models import NewsFeed | ||
|
|
||
|
|
||
| class FeedRedirectMiddleware: |
There was a problem hiding this comment.
you could also implement this inside the serve() of RootPage, that would only do the check when it is actually necessary and not with every request
There was a problem hiding this comment.
But that would increase coupling. Currently disabling/removing the app is very easy, it is never mentioned / used in core. Your thoughts on this?
There was a problem hiding this comment.
In my opinion, it would be okay to introduce some change to core, as myHPI (this application) is developed specifically for our myHPI website and not meant as a generic CMS. Hence, we don't need to be able to deactivate/remove the newsfeed app per installation and that small amount of coupling would be acceptable.
However, I'm not that familiar with Django patterns, so I can't really argue on whether there is even a need to replace this middleware or not.
There was a problem hiding this comment.
@lukasrad02 Felix and I had a conversation about the conflict of cool new features vs. size of codebase/maintainability. I made the argument that as long as fancy features are loosely coupled, future maintainer could easily remove them without the need to understand/update/fix them and therefore delay or block the maintenance of the core app.
The middleware is probably fine, as it doesn't do much. I just thought about it because we always struggle with pageload times and this is code that is not necessary on all pages but the root page.
Another way that we could pursue this is by adding some kind of "plugin" mechanism. Again citing ephios: we are sending out custom django signals to "plugins" (other django apps within the same repo or installed separately) and let them inject stuff at various places. e.g. there is a signal that says "hey plugin, give me some html to append to the homepage if you want to". This way, the additional code is only run when it is necessary.
There was a problem hiding this comment.
I'm fine with you two deciding on the best approach to do this. Especially when it comes to performance or plugins in Django, I would not be able to give any qualified arguments.
| post_key = models.CharField(max_length=255) | ||
| name = models.CharField(max_length=255) | ||
|
|
||
| icon = models.ImageField(upload_to=upload_avatar_to, null=True, blank=True) |
There was a problem hiding this comment.
is this the correct reference to also use all of the wagtail stuff for images?
There was a problem hiding this comment.
This is the same code that is used for uploading user avatars in Wagtail. Does not have to deal with collections etc., which makes sense because the post accounts should only be edited by admins anyway.
|
As an idea on how to handle this on the main page: Provide the first 3-4 news items (I think we could go for a two-column layout there and maybe even make the design a bit more compact) and add a link to "Further News" or similar. |
|
Regarding the Automatically write to feed on release feature: Do you think it would make sense to integrate this into the myHPI source code instead of the GitHub pipeline? I would think about some logic that checks the current version on startup, compares it with the version from the last startup and fetches the release description from GitHub if the version has changed. I would prefer this over creating the post from the pipeline, as there are usually a couple of hours or days between the release on GitHub and deploying it in production. Having some large post advertising new features on the homepage although these features are not even available might confuse users. |
|
Your reasoning makes sense to me. I wonder how that could be implemented. Especially, it is not guaranteed that there are not two releases between restarting myHPI. So basically, on start:
I'm not sure how to do the diffing. We could of course just do this by title or something like that, seems a little flaky as both github releases and news feed posts are not guaranteed to keep their title. Otherwise another structure would need to be created to store the mapping of release to post for diffing purposes. |
|
You've got a good point. I was thinking about comparing the version numbers, but this would of course require us to store them in addition to the posts, like the separate structure you've mentioned. I don't have a strong opinion on whether to introduce such a structure or not, but am slightly in favor of doing this. Otherwise, we would even have to differentiate between release posts and other posts during diffing, so there will be a lot of edge cases to consider. |
|
As another Idea: Why don't we pull the releases from GitHub in the action and place it in a file in the docker container? |
|
I think all of your variants are quite complex. I would prefer just using the tag of the current version (which we have easy acccess to from the python code) and creating a post for that by pulling the corresponding release notes if it does not exist yet. |
|
From my perspective, both the suggestions from Aaron and Julian would be fine |
|
Now implemented @jeriox 's version. the only problem I see here is that some releases may not be published. I.e.
To fix this, an admin needs to manually publish the post with Coming back to how the user is presented the feed. What I like about the current version is that no changes are done to core. I am not sure if that would be possible when including the posts on the root page. If that is not relevant, I think it should be possible. |
Nice solution!
Added some comment on this to the discussion above. |
4 participants
Close #479
TODO
Optional todos (can also be followups)
Questions
@jeriox we can do the election release first and merge this afterward if there were no problems