Aggregator module - add an option to not generate blocks [#43245]

Comment	File	Size	Author
#35	aggregator-block-35.patch	7.38 KB	mustafau

#34	aggregator-block.patch	6.23 KB	Dries

#32	aggregator-disable-block-43245-32.patch	5.75 KB	mustafau

#30	aggregator_block_3.patch	6.92 KB	agentrickard

#28	aggregator_block_2.patch	7.59 KB	agentrickard

#23	aggregator_block_1.patch	7.5 KB	agentrickard

#21	aggregator_block_0.patch	7.42 KB	agentrickard

#20	agg_0.patch	7.04 KB	agentrickard

#11	aggregator.module.patch43245b	5.18 KB	markus_petrux
#1	aggregator.module.patch43245a	7.85 KB	markus_petrux

Comment #1

markus_petrux CreditAttribution: markus_petrux commented 4 January 2006 at 06:43

Status:

Active

» Needs review

File	Size
aggregator.module.patch43245a	7.85 KB

Well, I have finally managed to do something like option 3. The attached patch worked for me.

It adds a new option to the aggregator settings panel that looks like this:

Block generation:
( ) Never, I do not wish to generate aggregator blocks.
(*) Always, please. I wish to generate blocks for all categories and feeds.
( ) Let me choose which categories and/or feeds I wish to generate blocks.

If you choose "let me choose...", then when you create/edit a category or feed you'll have a new option that reads "Create a block for this " that allows you to enable/disable this block, so it is hidden from block administration.

There is no change in the SQL schema in this patch. To store the enabled/disabled status for each block, it uses the 'block' column of the aggregator category/feed tables (set to 0 when disabled). Note this column is used to store the number of items to show for each block, so I believe the patch doesn't break anything.

Please, let me know what do you think. Thanks

Log in or register to post comments

Comment #2

Dries CreditAttribution: Dries commented 4 January 2006 at 15:23

Personally, I think option #2 would have been easier/simpler.

I'm not sure this feature is necessary. Is this a "real" problem?

Log in or register to post comments

Comment #3

markus_petrux CreditAttribution: markus_petrux commented 4 January 2006 at 15:32

IMHO, yes it is. Specially when you have defined lots of sources, the block administration gets kind of heavy.

The patch turned to be much simpler than I originally thought. By default, it would behave as always, but with it, you have options that may make your life somehow easier.

Log in or register to post comments

Comment #4

bluecobalt CreditAttribution: bluecobalt commented 4 January 2006 at 17:32

+1 on this.

I agree that the admin/blocks page gets unwieldy when there is a large number of feeds. It takes a long time to load, and you have to scroll through a lot of unneeded blocks to find the ones that you actually want to turn on.

I haven't tested this as I don't have 4.7 installed, but I like the concept.

Log in or register to post comments

Comment #5

markus_petrux CreditAttribution: markus_petrux commented 8 January 2006 at 18:41

Thanks, bluecobalt. It's good to know I'm not the only one. :-)

It would be nice if someone else (other than me) could test it. Otherwise it won't get into HEAD, I'm afraid.

The patch simply adds the ability to choose when/how you wish to generate blocks for aggregator feeds with no changes in the DB at all. It uses an existing field, "block" that's used to set the number of items to show per block. With the patch, if this field is set to zero, the block won't appear in block administration. Oh, ah, and there's also a global variable that allows you to say if you want blocks "always", "never" or "let me choose". Pretty simple, really. ;-)

Log in or register to post comments

Comment #6

chx CreditAttribution: chx commented 8 January 2006 at 23:59

I know posts like this is not much help but I do not have the time to review but definitely Ó1 for this patch. I put an underscore before aggregator_block function name to kill it quick...

Log in or register to post comments

Comment #7

eaton CreditAttribution: eaton commented 9 January 2006 at 00:14

The functionality is VERY VERY useful, though -- automatic generation of blocks is nice for a blogroll and horribly bad for large-scale aggregation.

+1 for functionality

Unfortunately, it doesn't seem to apply correctly.

patch: **** malformed patch at line 39: else if ($op == 'configure') {

Log in or register to post comments

Comment #8

markus_petrux CreditAttribution: markus_petrux commented 9 January 2006 at 06:42

>> malformed patch

Not sure how to check this. I have little experience generating patches, I used WinMerge to generate this one, based on $Id: aggregator.module,v 1.268 2005/12/29 04:46:40 unconed Exp $.

:-(

Log in or register to post comments

Comment #9

Dries CreditAttribution: Dries commented 9 January 2006 at 10:20

I'd kill the global option and add a per feed option. I think that is going to be the least confusing.

Log in or register to post comments

Comment #10

markus_petrux CreditAttribution: markus_petrux commented 14 January 2006 at 18:19

Let me bump this one as a self reminder. As soon as this patch gets applied (or rejected <g>), I'll work on it (in the hope to avoid conflicts with the patch format).

Log in or register to post comments

Comment #11

markus_petrux CreditAttribution: markus_petrux commented 16 January 2006 at 18:23

Status:

Needs review

» Reviewed & tested by the community

File	Size
aggregator.module.patch43245b	5.18 KB

I'd kill the global option and add a per feed option.

ok, the new patch just adds an option to the aggregator category/feed to choose if a block is desired by that specific item.

If the user says yes, the default of 5 items per block is stored in the block field of the corresponding table.
If the user says no, the block field is set to 0.

Then, then block hook query (where block are generated) has been modified to list items where the block field is greater than 0.

Please, see attached patch. I hope this one works. ;-)

Log in or register to post comments

Comment #12

Dries CreditAttribution: Dries commented 17 January 2006 at 15:49

The way you added the options is ... weird. Some might argue it is a clever hack but to me it's somewhat ugly.

Log in or register to post comments

Comment #13

markus_petrux CreditAttribution: markus_petrux commented 17 January 2006 at 18:21

I tried to find a way to implement this option by not touch the DB tables. Do you mean, it would be best to add a new field?

...or it is related to the way I added the fields in the form? Is not a good option to use a checkbox (boolean) that updates a numeric column in the tables?

Sorry, what do you mean by "somewhat ugly"? :-( ...how would you do it?

Log in or register to post comments

Comment #14

markus_petrux CreditAttribution: markus_petrux commented 21 January 2006 at 12:49

Please, let me know what's wrong, so it be could be properly addressed.

1) Is it better to use a different table column to store the "generate block state"?
2) Is the way form fields are validated? Maybe something more simple here?
3) ???

Log in or register to post comments

Comment #15

Dries CreditAttribution: Dries commented 22 January 2006 at 08:34

I'm leaning toward saying that an extra column field is nicer code-wise.

Log in or register to post comments

Comment #16

Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commented 27 March 2006 at 10:53

Status:

Reviewed & tested by the community

» Needs work

as per Dries' comments

Log in or register to post comments

Comment #17

markus_petrux CreditAttribution: markus_petrux commented 8 April 2006 at 16:07

Bumping just to not forget this. I'll look at it.

Log in or register to post comments

Comment #18

Egon Bianchet CreditAttribution: Egon Bianchet commented 17 January 2007 at 20:35

Version:

x.y.z

» 6.x-dev

Resurrecting ...

Log in or register to post comments

Comment #19

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 15 March 2007 at 20:52

Subscribing. Will take a look as I need this functionality for to the MySite module project.

I was already considering this patch. Needs three things?

1) A new column in the aggregator_feed table to indicate block status
2) A global default setting that the admin defines (Block on/off).
3) A per-feed radio form element that defaults to the admin settings (on/off), but allows changes on a per-feed basis.

Log in or register to post comments

Comment #20

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 25 March 2007 at 21:35

Status:

Needs work

» Needs review

File	Size
agg_0.patch	7.04 KB

New patch against 6.x-dev.

The administrator sets a global option for default settings. Individual feeds can add/remove their block on update.

Perhaps needs a permission check to see if the person editing the form has permission to create blocks?

Also includes a db schema change in aggregator.install, adds a 'block_create' column.

Log in or register to post comments

Comment #21

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 25 March 2007 at 22:27

File	Size
aggregator_block_0.patch	7.42 KB

Improved version of previous 6.x-dev patch. Now respects 'administer blocks' permission.

Log in or register to post comments

Comment #22

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 28 March 2007 at 14:47

The last patch has a logic error in the block_rehash values set in the form. Will fix and post a new patch later today.

Log in or register to post comments

Comment #23

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 29 March 2007 at 01:29

File	Size
aggregator_block_1.patch	7.5 KB

This should be ready to go.

Log in or register to post comments

Comment #24

m3avrck CreditAttribution: m3avrck commented 29 March 2007 at 19:45

I always used to hack this out too.

I would like to see a follow up patch that makes aggregator actually use the taxonomy instead of it's own crude system. That kills the menu system.

With both of those fixes, aggregator will then only *slightly* suck ;-)

+1

Log in or register to post comments

Comment #25

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 30 March 2007 at 00:04

Ted-

With a little thought, we could extend the proposed Aggregator API in two ways:

1) Let the admin select Taxonomy or Category for term storage
2) Let the admin select the parsing engine -- assuming that there is a default engine, and that contrib modules like simplefeed or leech have different parsing rules (ie. turn items into nodes).

Why don't you add notes to http://drupal.org/node/130942 and let's close this issue after its RTBC.

Log in or register to post comments

Comment #26

Dries CreditAttribution: Dries commented 30 March 2007 at 08:24

Status:

Needs review

» Needs work

Looking at the aggregator module's code, it looks like there is already a aggregator_feed.block field. It seems to specify the number of news items in a block, and the allowed values range from 2 to 20. I wonder if we can't add a special case for that and allow 0 to mean 'no block'. Not sure what would be best.

I'm also -1 on the global configuration setting. That's just going to be very confusing.

Another problem with this patch is that it doesn't provide an upgrade path. It looks like existing blocks might get turned off after applying this patch (or after upgrading to Drupal 6).

Log in or register to post comments

Comment #27

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 30 March 2007 at 19:57

Three points here:

1) Well, back at http://drupal.org/node/43245#comment-67551, you suggest that an extra field is best, so I went that way.

This feature could be done either way. Pick one; the code is easy. That column becomes either 5 or 0. Or we add the new "block_create" column.

2) As for upgrade path, if we make the default setting "1" == 'on' == '5' (number of items) instead of '0' == 'off', then old blocks will not be destroyed by the upgrade. Right?

I think setting the default value to "on" (however that is stored in the db) fixes the upgrade issue, since existing blocks are only destroyed if they go from "on" to "off".

3) Personally, I don't find the global setting confusing. From a use-case, it's exactly the functionality that I want.

I'll fight for #3, I think that the implemetation is different, but optimal because it provides the most administrator flexibility. Without a global setting, I have to manually select block status for each feed.

Plus, there are use cases (though rare), where an administrator might have 'administer feed' permissions but not 'administer block' permissions. The global setting accounts for that case.

Log in or register to post comments

Comment #28

agentrickard

he/him

English

Georgia (US)

CreditAttribution: agentrickard commented 31 March 2007 at 18:07

Status:

Needs work

» Needs review

File	Size
aggregator_block_2.patch	7.59 KB

Revised patch. The change is that the default setting is now '1' or 'on'. This means that upgraded installations will keep existing blocks, and the module behavior will remained unchanged unless the administrator explicitly turns off block creation.

There is still a 'default setting' for the admin and a setting per feed option, as per my previous note.

Also still storing in a separate column, as the argument for dual-use on the 'block's column seems a little hack-like.

Log in or register to post comments

Comment #29

Boris Mann CreditAttribution: Boris Mann commented 6 April 2007 at 23:19

+1 on global defaults. Dries, think about it in terms of *lots* of feeds. Especially if OPML Import gets in, then you would have to afterwards go through each feed manually.

See http://groups.drupal.org/node/3538 for an overview of lots of things to fix with aggregator.