Page 1 of 1

Album Category bugfix

Posted: Sun Aug 05, 2007 12:45 pm
by b0n3m4n
I found a bug in the album module and would like to present a fix. I don't have much time for testing so I'll just post my findings here instead of submitting a proper patch. I'll post a copy in the dev forum.

MODULE: Album 0.9.2

BUG: Create a category, add a few albums (at least 3), click the up-arrow for the lowest album to move it up. The album will move up to the top instead of just one place up. This bug includes other reordering issues and this is only one example that's easy to reproduce.

REASON: In action.updatealbum.php the function ListingCount from Album/classes/module/class.Album.php is called which due to an error in implementation always returns 0. Thus in the table cms_module_album_category_listings the column listing_number is always initialized as 1 which leads to severe reordering problems.

SOLUTION: change 2 places in 2 functions:

1) Album/action.updatealbum.php line 40

Code: Select all

$listing_number = $this->ListingCount($album_id)+1;
needs to be changed to

Code: Select all

$listing_number = $this->ListingCount($category_id)+1;
2) Album/classes/module/class.Album.php line 590, function ListingCount

Code: Select all

function ListingCount($album_id)
{
	global $gCms;
	$db = $this->GetDb();
	$query = 'SELECT listing_id FROM '.cms_db_prefix().'module_album_category_listings WHERE listing_album_id='.intval($album_id);
	$dbresult = $db->Execute($query);
	if ($dbresult)
		return $dbresult->RecordCount();
	else
		return 0;
}
The sql-query is erroneus, the function should be like this:

Code: Select all

function ListingCount($category_id)
{
	global $gCms;
	$db = $this->GetDb();
        $query = 'SELECT MAX(listing_number) FROM '.cms_db_prefix().'module_album_category_listings 
                WHERE listing_category_id='.intval($category_id);

	$dbresult = $db->Execute($query);
	if ($dbresult)
	   {
  	           $row = $dbresult->FetchRow();
	           return $row['MAX(listing_number)'];
	   }
	else
	    return 0;
}
I guess the function should be renamed as well, as it does not count the entries of a category anymore. I have tried fixing the query so it does a correct listingcount but that lead to other problems. I.e. if list has 10 entries and list entry 9 is updated then the function will delete entry 9 (as it should), then it counts the number of entries (9) and gives the updated entry the count+1 (10) so that now two entries with the number 10 exist. My solution does not count the entries at all but determines the maximum number prevalent in the entries. Each new entry gets a number that is 1 greater than the current largest number.

I'd be glad if anyone could double check this implementation (it works for me so far) and post any anomalies.

Cheers,
Cerno

Re: Album Category bugfix

Posted: Mon Aug 06, 2007 5:10 pm
by Elijah Lofgren
I'm cross-posint what I posted here: http://dev.cmsmadesimple.org/forum/mess ... sg_id=1598 below ;)

> I suggested a solution to that bug here:
http://forum.cmsmadesimple.org/index.ph ... 040.0.html

Hi Jan,

Thank you very much! :)

I've given you commit access to Album and I'd appreciate very much if you could commit this fix directly to SVN as I am really busy with other stuff these days. ;)


Assuming you are using Windows, I would first download TortoiseSVN:
http://tortoisesvn.tigris.org/ and then svn checkout
http://svn.cmsmadesimple.org/svn/album/trunk

Then just paste in your changed files over the ones you checked out
and right click and do an SVN Commit with TortoiseSVN.

If you'd rather not commit this to SVN yourself, I'll hopefully try getting your fix committed this weekend.

Thanks!  :)

Have a great day,

Elijah Lofgren

Re: Album Category bugfix

Posted: Mon Aug 06, 2007 6:28 pm
by b0n3m4n
Hi Elijah!

I'm sorry but I'm going on vacation tomorrow, so I'll be away for at least 2 weeks. When I come back I might find the time to commit the code but better not count on it. I'd be glad to help, but knowing me I'll simply forget.

Let's continue from here 2 weeks from now :)

Cheers,
Cerno

Re: Album Category bugfix

Posted: Mon Aug 06, 2007 8:53 pm
by Elijah Lofgren
I've added looking into this to my CMSMS todo list: http://www.rememberthemilk.com/home/eli ... 2424/  and set the date at two weeks from tomorrow ;)

Re: Album Category bugfix

Posted: Tue Aug 21, 2007 2:11 pm
by Elijah Lofgren
b0n3m4n wrote: I'm sorry but I'm going on vacation tomorrow, so I'll be away for at least 2 weeks. When I come back I might find the time to commit the code but better not count on it. I'd be glad to help, but knowing me I'll simply forget.

Let's continue from here 2 weeks from now :)
Hi Cerno,

Let me know if you have time to commit the code. ;) Otherwise I will try to do it before I go back to college on Sunday. :)

Thanks,

Elijah

Re: Album Category bugfix

Posted: Thu Aug 23, 2007 2:06 pm
by rtkd
hmmm, i applied the above changes but nothing changed? *confused*

Re: Album Category bugfix

Posted: Tue Sep 04, 2007 8:42 am
by rtkd
any news on this one?

Re: Album Category bugfix

Posted: Mon Sep 24, 2007 7:05 pm
by nicmare
same problem here. bug is not fixed with the changes!

Re: Album Category bugfix

Posted: Sat Oct 20, 2007 8:09 am
by Roland
Having the same problem, I manually corrected the table cms_module_album_category_listings with phpMyAdmin
for already existing albums (in one category).... then it worked.

Field: listing_number to 0,1,2,3,...
(without the patch this field was not set correctly when album was created/added to the category)

Re: Album Category bugfix

Posted: Sat Dec 22, 2007 6:31 am
by Elijah Lofgren
First off, sorry for my unbearable slowness in responding to and dealing with this issue. I focus almost entirely on my studies during the semester which leaves almost no time for CMSMS. ;)

This bug has been fixed in r336:
------------------------------------------------------------------------
r336 | elijahlofgren | 2007-12-22 00:11:18 -0600 (Sat, 22 Dec 2007) | 6 lines

Fix bug: "[#1524] Reordering is broken" Reported here: http://dev.cmsmadesimple.org/forum/mess ... sg_id=1378 and partially fixed here: http://forum.cmsmadesimple.org/index.ph ... 040.0.html
A big thanks to Cerno for tracking down the source of this problem and supplying a partial fix!
Note: In order to make reordering work after upgrading, you must first unlist all Albums from
their categories (uncheck all "Category Listings:" for each Album) and *then* add the Albums back into the categories. This
will make the Albums be listed in the categories correctly so that they can be reordered properly.

------------------------------------------------------------------------
The fixed files can be downloaded here:
http://svn.cmsmadesimple.org/svn/album/ ... ealbum.php
http://svn.cmsmadesimple.org/svn/album/ ... .Album.php

Just overwrite your current modules/Album/action.updatealbum.php and modules/Album/module/class.Album.php files with those.


But note (from the commit message):

"In order to make reordering work after upgrading, you must first unlist all Albums from
their categories (uncheck all "Category Listings:" for each Album) and *then* add the Albums back into the categories. This
will make the Albums be listed in the categories correctly so that they can be reordered properly."


Thanks Cerno for your help in fixing this bug!  :)

Roland wrote: Having the same problem, I manually corrected the table cms_module_album_category_listings with phpMyAdmin
for already existing albums (in one category).... then it worked.

Field: listing_number to 0,1,2,3,...
(without the patch this field was not set correctly when album was created/added to the category)
Thanks Roland for finding out how to fix existing albums. This is an alternative to unlisting all the albums from their categories and then relisting them (after overwriting the 2 files with their fixed versions).

I leave on vacation tomorrow but I'll try to check back here later on in case there are any questions.

I hope everyone has a very Merry Christmas,

Elijah Lofgren

Re: Album Category bugfix

Posted: Sun Feb 03, 2008 12:01 pm
by Gregor
Hello,

I update the two php-files as mentioned. I realy would like to sort the shown albums by Album-id. The way it is sorted by now, does not show logical to me. I expect, based on the following tag, the newest album shown first

Code: Select all

{cms_module module="album" lang="nl_NL" sortdesc="true"}
Changing true to false does not help.
http://www.uisge-beatha.eu/index.php?page=fotoalbum

Any help on this one is gretly appreciated!

Thanks for your time,
Gregor

Re: Album Category bugfix

Posted: Sun Feb 03, 2008 7:32 pm
by Elijah Lofgren
Gregor wrote: Hello,

I update the two php-files as mentioned. I realy would like to sort the shown albums by Album-id. The way it is sorted by now, does not show logical to me. I expect, based on the following tag, the newest album shown first

Code: Select all

{cms_module module="album" lang="nl_NL" sortdesc="true"}
Changing true to false does not help.
http://www.uisge-beatha.eu/index.php?page=fotoalbum

Any help on this one is gretly appreciated!

Thanks for your time,
Gregor
Hi Gregor,

By default Album shouldn't use category mode to display the Albums so calling album with:

Code: Select all

{cms_module module="album" lang="nl_NL" sortdesc="true"}
shouldn't make any difference whether the files are modified.

It seems to work fine on my site: http://www.elijahlofgren.com/album-test/

I'm not sure why it's not working yours. And I need to get to work on homework.

Sorry I can't be of help,

Elijah

Re: Album Category bugfix

Posted: Sun Feb 03, 2008 9:18 pm
by Gregor
Thanks for your reply Elijah. I think I found the solution.
The table lay-lout:
   
rijen beginnend bij
in modus en herhaal kopregels na cellen
 
Sorteren op sleutel:
Volledige teksten album_id album_name album_number thumbnail_path comment column_number row_number template
Wijzigen Verwijderen 1 Koop 12 /Koop/thumb_Nikon 025.jpg NULL 5 4 Thickbox-with-next-prev-links
Wijzigen Verwijderen 3 Zeilen met Rob 7 /Zeilen met Rob/thumb_P1000860.JPG NULL 5 4 Thickbox-with-next-prev-links
Wijzigen Verwijderen 4 Voorbereiding 11 /voorbereiding/thumb_Buitenkant.jpg NULL 5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 5 Workum naar Amsterdam 10 /Workum_Amsterdam/thumb_Haven_Enkhuizen.jpg NULL 5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 6 Amsterdam naar Hellevoetsluis 9 /Amsterdam_Hellevoetsluis/thumb_Genaker_hijsen.jpg NULL 5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 7 Hellevoetsluis naar Herkingen 8 /Hellevoetsluis_Herkingen/thumb_Gaan_we_of nog_nie... NULL 5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 8 Setup Genaker 6 /Setup_Genaker/thumb_Setup_Genaker6.jpg NULL 5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 9 Richting winterberging 5 /Winter2006/thumb_Winter2006_15.jpg   5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 10 Boiler 4 /Winter2006/Boiler/thumb_boiler-1.jpg ... weggewerkt, of toch niet....
5 3 Thickbox-with-next-prev-links
Wijzigen Verwijderen 13 Herkingen-Ramsgate 3 /Ramsgate/thumb_Herkingen-Ramsgate14.jpg NULL 5 0 Thickbox-with-next-prev-links
Wijzigen Verwijderen 14 Zeilweekendje Grevelingen Zierikzee 2 /Zeilen_met_mafv/thumb_Zeilweekend_01sept07_41.jpg Het was een bewogen weekend 5 0 Thickbox-with-next-prev-links
Wijzigen Verwijderen 15 Clubwedstrijd sept.2007 1 /Clubwedstrijd/thumb_wedstrijd0709-4.jpg NULL 5 0 Thickbox-with-next-prev-links
Wijzigen Verwijderen 18 Zeilen in de Hagel 13 /Hagel_Schaken/thumb_Hagel en leren schaken-1.jpg NULL 5 0 Thickbox-with-next-prev-links
Met geselecteerd: Selecteer alles / Deselecteer alles Met geselecteerd: Veranderen Verwijderen Exporteer
Sorry, was not able to make a screenshot.

It is/was ordered by album_number. I changed the "order by" statement to use album_id

Code: Select all

function GetAlbums ($ids=false, $sortdesc=false, $category_id=false)
{
	global $gCms;
	$db = $this->GetDb();
	$config = $gCms->config;
	
	if ($category_id == false)
	{
		if ($ids===false || $ids=='')
		{
			$query = 'SELECT * FROM '.cms_db_prefix().'module_album_albums ORDER BY album_id';
			// was album_number
		}
		else
		{
			$query = 'SELECT * FROM '.cms_db_prefix().'module_album_albums WHERE album_id IN ('.$ids.') ORDER BY album_id';
			// was album_number
		}
	}
	else
	{
By now it sorts ok for me. I don't know why there is a diference in the numbers of album_id and album_number.

suggestion to have someone put this into the svn-version of the Album module?

Gregor