Forum OpenACS Development: Accidently updated news-aggregator => Big Pain...

1: Accidently updated news-aggregator => Big Pain...

Posted by Malte Sussdorff on 01/23/06 04:45 PM

News-Aggregator update does not take into account that previous versions of news-aggregator can run on .LRN.

It therefore decides to drop the package_id, assuming that package_id is magically set in acs-objects in 5.2, which is partially true, but not on upgrade. I commented this out, so noone comes into the pain of looking into the backup file and manually setting the package_id of the acs-object, as this is needed by the news-aggregator-portlet.

Sadly now I'm stuck as I can't add any new sources as it demands an aggregator_id. MichaelS, could you enlighten me what is needed to create a source or even better, fix news-aggregator-portlet so it allows you to create new sources?

2: Re: Accidently updated news-aggregator => Big Pain... (response to 1)

Posted by Michael Steigman on 01/23/06 06:01 PM

I didn't change the required flags for the new source proc (certainly didn't add aggregator_id). A lot of work had gone into the package since the last tagged release so it's quite possible that was done a while back and you're just now seeing it because of the new releases.

In any event, sources should not be tied to packages as they may be used across instances and that bit of RI makes life really tough when you try to drop sources that are no longer in use (to avoid chewing up CPU cycles and database rows on pointless updates) or drop packages. One could argue they shouldn't even be objects (no need for permissions, either) but after talking about this with Dave on IRC, I decided to leave them as objects for now in case we ever wanted to add categorization to the feeds.

Back to the point - unfortunately, I don't have a .LRN install to test on so I can't work directly on the portlet. After taking a peek at the aggregator portlet code, I think it might simply be a problem with the URL being used to access the subscriptions page. Are you using the repository or 5.2 branch? I'll try to check in a change that'll fix it and you can let me know if it works.

3: Re: Accidently updated news-aggregator => Big Pain... (response to 1)

Posted by Michael Steigman on 01/29/06 06:01 PM

At this point, though, if you are using the package in a .LRN environment you should not upgrade as Malte's diagnosis is correct. Unfortunately, the portlet depends on a piece of the data model which no longer serves a purpose in the package and in fact is detrimental to its progress. I think that the best course of action at this point is to fix the portlet so that it works in a sensible way does not hamstring further development of the package. Since releasing another version of the package with Malte's comment in the upgrade script would cause data model inconsistencies, I have removed it with the hope that interested parties can collaborate on a fix for the portlet.

Moving forward, if there are any users or developers who would like to work with me to ensure that the portlet continues to function, that would be great.

4: Re: Re: Accidently updated news-aggregator => Big Pain... (response to 3)

Posted by Malte Sussdorff on 02/03/06 12:18 PM

Michael, your datamodell change dropped a column without putting the data stored in this column somewhere else. And sad as it might sound, the values stored in the column are crucial. So putting a drop statement in the code is bad.

I do agree with your point that if we store the package_id in the acs-object we should not store that package_id with the na_sources table. But dropping the package_id for easier coding is not the way to go. Please provide an upgrade statement that puts the package_id into the acs-objects before dropping the table.

5: Re: Accidently updated news-aggregator => Big Pain... (response to 1)

Posted by Nima Mazloumi on 02/03/06 12:51 PM

Is this problem still there when checking out from oacs-5-2 and trying an upgrade?

6: Re: Accidently updated news-aggregator => Big Pain... (response to 1)

Posted by Dave Bauer on 02/03/06 04:25 PM

Malte,

The idea that a source belongs to only one instance of news aggregator is wrong. A source could be associated with any instance of news aggregator, so if for instance, I remove on instance of news aggreagtor, the source could still be used by other instances, and the row should not be removed when the package instance is removed.

7: Re: Re: Accidently updated news-aggregator => Big Pain... (response to 6)

Posted by Michael Steigman on 02/03/06 05:03 PM

Exactly. So, no, I'm not going to write that upgrade script because it will break the package as it works now. Also, for clarification, the package_id column in na_sources was NOT being used in new installations - every row in my na_sources table has a null in the package_id column. It appeared useless and confusing. I haven't dug through the CVS logs to see who made that change and why but the bottom line is that there was no reason to believe dropping the column would have any effect.

I haven't worked with the portlet architecture but I think the right thing to do would be to add an aggregator_id attribute to the portlet and let the portlet query the DB the same way the full package does, i.e., pull all items belonging to the aggregator_id and sort by date, then source. I'm willing to help sort this out but as I said above, I don't have a portal or .LRN installation to work on.

Nima - yeah, it's still going to be an issue. You can avoid the problem by avoiding the upgrade or by running the 0.9.7-0.9.8 upgrade script by hand and omitting "alter table na_sources drop package_id".

8: Re: Re: Accidently updated news-aggregator => Big Pain... (response to 6)

Posted by Malte Sussdorff on 02/04/06 08:07 AM

Dave, Michael, I totally agree with you. But dropping a column in a databse script without making sure that the data in the column is preserved is bad. Seriously. I don't care about news-aggregator. We don't use it and I got this to work at a client site after upgrade. My only point is: Don't drop database columns *unless* you provide an upgrade script which stores the column value somewhere else.

Michael, can you answer me the question why it is mandatory that the package_id is dropped from na_sources? It obviously will break *any* .LRN upgrade that is using news-aggregator. Whereas if we just keep the package_id, news-aggregator would still run.

9: Re: Accidently updated news-aggregator => Big Pain... (response to 1)

Posted by Dave Bauer on 02/04/06 02:11 PM

Malte,

The .LRN integration has a bug if it relies on na_sources.package_id. The column is totally meaningless. The .LRN code for news aggregator needs to be fixed to associate an aggregator to a class instead of a source.