Closed Drumor closed 5 years ago
Let's not consider the CI output, I am unsure what is going wrong there. I have ran the test suite locally and found no bugs.
Nonetheless, I don't think it is wise to include plugin-specific code into the core of ICTV, i.e. the if channel.plugin.name == "editor"
line.
Here is a list of alternatives ways I would rather be in favor of for fixing this email-spamming problem. They are sorted by decreasing complexity.
1) Make plugins smarter:
Template
class has an helper methods to check for non-complying fields. Plugins have the tools to deal with this on their one.2) Fix the channels configurations that produce non-complying slides (for RSS plugin only most probably).
3) Add a plugin parameter to disable emails for a particular plugin.
4) Add a configuration parameter (i.e. in the configuration.yaml file) that blacklists plugins from the sending of these emails.
In the long term, I would prefer option 1. I think 4 is not really appropriate, one doesn't need to edit the configuration file when installing plugins, let's keep it that way. Option 3 is may be the way to go if you are lacking of time. Note that you can combine the options for an even better result.
Hello, Maybe a problem from my pull request? I had some trouble with the initial git clone (dunno how). Ok for the alternatives. I'll propose the first implementation as soon as possible. I understand your argument about including plugin specific code into the core. Thx for the review :-)
I have made an update of the pr base on the point 1 of your comment @mpiraux . I didn't add this into database.py:
if db_version <2: print('Updating database to version %d' % 2) column_sql = PluginChannel.sqlmeta.getColumns()['drop_silently_non_complying_slides'].sqliteCreateSQL() table = PluginChannel.sqlmeta.table assert conn.queryOne('ALTER TABLE %s ADD %s' % (table, column_sql)) is None db_version = 2
Dunno if it is necessary as this is necessary only for user that already have an instance of ictv before this PR.
I think you should. UCLouvain SGSI has already deployed this version of ICTV. Could you also merge the master branch with the Travis fix into this one ?
I agreed with your remarks.
It was not clear to me at the beginning, but I think this parameter should be implemented as the Cache activated
and Cache validity (minutes)
parameters. I.e. it should have a default value per plugin, and then an additional parameter per channel to override it. You can take a look at how these two are implemented, I think your case should be very similar.
@mpiraux this one seems mergeable.
There is a lot of automatic emails that come to the admins but they have no possibility to edit datas because it's automatic channels. Limiting the email for editor channels seems a good option.