Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

When a category is removed, uncategorize feeds/categories explicitly to plugins #786

Merged
merged 1 commit into from
Nov 26, 2018

Conversation

39aldo39
Copy link
Contributor

@39aldo39 39aldo39 commented Nov 5, 2018

Currently, when a category is removed without children, its feeds/categories are uncategorized. However, these changes aren't sent to the plugins. For example, in Nextcloud the children are removed.

This pull request sends these changes explicitly to the plugin.

@brendanlong
Copy link
Collaborator

I'm not sure if this will work right if plugins support multiple categories per feed, although I'm guessing the original logic isn't right either. Do you know how to review this @jangernert ?

@brendanlong
Copy link
Collaborator

brendanlong commented Nov 22, 2018

Is it possible that this belongs at the plugin level, in OwnCloudNewsInterface.deleteCategory (under plugins/backend/owncloud), and the NextCloud plugin is just implementing that function incorrectly?

@jangernert
Copy link
Owner

jangernert commented Nov 22, 2018

Thx for your efforts with this PR :)

I would also suggest to move this into the NextCloud plugin. Most RSS services have children of deleted categories still around as uncategorized.

@39aldo39
Copy link
Contributor Author

I do think the implementation is correct when a feed has multiple categories. The function moveFeed only removes the given category from the feed.

Also, it is quite tricky to implement it in the plugin, since the database might update before the plugin method is called, in which case the children cannot be read from the database.

I mainly made the change for my fork with a plugin for syncing with my DecSync. For that plugin, it is way easier to implement the deleteCategory with this pull request. I thought of doing a pull request for that plugin next week. It also needed another change, a bit similar to this one.

@brendanlong brendanlong self-requested a review November 24, 2018 00:30
@brendanlong
Copy link
Collaborator

brendanlong commented Nov 24, 2018

Ok I looked at this a little more, and I'm still a little worried because this part of the code is pretty flaky, but I'm pretty sure this pull request won't make it any worse. Hopefully sometime soon we'll be able to test the plugins.

This seems to work in Feedbin (well.. as well as it worked previously, which is "barely"). Do you mind if I merge this @jangernert?

@39aldo39 I took a look at your fork and I'm happy to review a pull request to add your plugin. I'll try to review it later this week if I can find time, but the major thing that stands out is that we should try not to duplicate the dependencies shared with the local plugin (date parsing, libmrss, etc.).

@jangernert jangernert merged commit d02a805 into jangernert:master Nov 26, 2018
@39aldo39 39aldo39 mentioned this pull request Nov 27, 2018
@39aldo39 39aldo39 deleted the fix-remove-category branch December 26, 2018 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants