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

Feature: Adding default feeds #1255

Closed

Conversation

NicoWend
Copy link
Contributor

This pull request is a draft. We saw that there's a demand for the ability to, as admins, set default feeds for users. It seems to be valuable for the news app.

The goal is to present our vision and our ideas on how to implement it, in hopes of hearing your opinion, before we start to code.

🚀 The feature

This feature will allow admins to set default feeds for all users.

For now, as an admin, it's possible to add a feed for a user with a command line by using ./occ news:feed:add <user_id> <feed>. However, the command line allows to add a feed for only one user. Also, it's more user friendly to have a interface to do this. Furthermore, this is a prety highly request demand, as seen on this discussion.

The idea is to add a parameter that allows an administrator to define default feeds. These feeds will be added by default for all existing and new users on the server. This parameter will be modifiable on the settings page (settings/admin/news).

👨‍💻 Implementation

This new parameter will be added to the application configuration. On the front-end, there will be a list where admins will view default feeds and be able to add new ones, by adding their RSS URLs, or delete them. Each user on the server having the news app will have them by default.

The array of RSS URLs will be stored in the oc_appconfig in a string in the JSON format.

We have analyzed the code and here's a solution we would like to propose.

1️⃣ Adding the new parameter

  • Add new parameter in the config table (stored in oc_appconfig )
  • Add input in front-end
  • Send requests to the back-end to update the values

2️⃣ Adding and fetching defaults parameters for users

  • For new users, we create a hook (or EventDispatcher), that on each user creation event, creates and fetches the default feeds for that user.
  • For existing users, we will create & fetch default feeds for them when the parameter is modified.
    • We have also considered creating a hook which on each user login, checks if the user has the default feeds, and if not inserts them. This seems faster because we don't do everything at once. Also, users that are not active won't have the feeds inserted for them.
    • However, we're not sure if this is as clean as a solution as the first one, since it might be too much to check the feeds on each user login.
    • We'd like to hear your opinion and perspective before we start implementing this, so any comments are welcome!

Hope this will catch your interest!

Team Forward,

  • Marco NASSABAIN (@mnassabain)
  • Aurélien DAVID
  • Jimmy HUYHN
  • Hamza ELHADDAD
  • Ilyes MALIH CHERGUI
  • Nicolas WENDLING (@NicoWend)

@codecov-io

This comment has been minimized.

@SMillerDev
Copy link
Contributor

I don't think this is something the news app should be providing. It's becoming more of a general information board if we provide this feature and I don't think that is a worthwhile addition.

@Grotax or @anoymouserver if you think it is feel free to disagree with me.

@anoymouserver
Copy link
Contributor

I can understand @SMillerDev's reasoning, but I don't really have an opinion.

From my point of view, there are already some kind of 'default feeds' that are suggested to the users, which can then be added to the subscribed feeds (namely the 'Explore' section), which also can be customized.
I don't see any real use case for which it would be necessary to distribute feeds to all users as an admin, which would not also be possible via the 'Explore', leaving the actual decision to the users.
In the end, it causes the database to grow unnecessarily because the same feed is retrieved for everyone over and over again, even if the app is not used by this user, because then they are not read and therefore deleted.

@SMillerDev
Copy link
Contributor

Just looked up if other nextcloud apps provide a way to show everyone the same information/feed and there is https://apps.nextcloud.com/apps/announcementcenter for that.

@Grotax
Copy link
Member

Grotax commented Mar 24, 2021

Hey so I have been thinking about it for a while and I came to the conclusion that I don't want this in news.

The reason behind is that news in itself is a feed reader, with an API for clients. It's main purpose is to enable you to read news without relying on third party services.

I can understand the wish behind this functionality but I think the target is, they want to inform users as an admin. Imagining a school or some company, people login and see the announcements my the principle or their manager...

And yes this target could be potentially reached with news but it's like my professor for software engineering used to tell us, if you only have a hammer then the whole world is a nail (roughly translated)

Additionally I think that news is not feature complete, the basic stuff barely works and the two main developers invest a lot of time to get it working. Then there is the V2 API which is much smarter by design but not implemented yet.
On top of that I think news could use some smart/machine learning/something something in the future.
To give an alternative to companies like Google that do that for you (Google news)

TL;DR I say no to this and I don't see any chance for this to land in news.

Thanks though for the wonderful PR its really nice to work with you guys. ☺️

@NicoWend
Copy link
Contributor Author

Hello everyone,
we've understood your position and we really wanted to thank you for your analysis and, for explaining us why it seems not a good feature for the app.

The pleasure is shared for working with you all, news is a really good app. 😃

@NicoWend NicoWend closed this Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants