Reviews have less methods compared to comments

[teraßyte]

Lately I've been going through all my old todo lists and code and I've been finding a lot of notes I made about reviews not having the same treatment as comments. One example is the other topic I made yesterday:

Another topic is the previous one (thought it is not exactly related to a missing function, but rather updating an existing one):

 

The point is that the reviews extension has less "default" methods available (I did notice another 2 but I can't remember the names right now), for example I have added in my own Item class the lastReviewer() method which does NOT exist while lastCommenter() is already available for the comments extension. Can we please get the same methods for both comments and reviews extensions? And I'm not talking about the current methods only, but also for methods added in the future to the core code. Right now it seems that comments are a higher priority compared to reviews, they should be equal imo

[teraßyte]

Here's another difference between comments and reviews, the comments per page count is a method that returns a number while the reviews per page count is a static variable:

	/**
	 * @brief	[Content\Item]	Number of reviews to show per page
	 */
	public static $reviewsPerPage = 25;

	/**
	 * Get number of comments to show per page
	 *
	 * @return int
	 */
	public static function getCommentsPerPage()
	{
		return 25;
	}

 

Can you make things more consistent in the future please? 

[Mark]

Yes, it's on our list. I'd actually like to make it more so rather than specifying a class for comments and/or a class for reviews you can specify any number of classes able to handle comments, of which one could be a review class.

[teraßyte]

While we're at it, you allow digests of items and comments but not of reviews. If I want to send out a digest email of daily/weekly reviews I can't.

[teraßyte]

Another one for the list: the stats() method in the _Item class returns only the number of comments and views, no reviews count. I had to extend the function in my own item class to return that too if a review class is available.

[teraßyte]

Here's another addition to the list while I am at it: getItemsWithPermissions() has an option to join the last commenter BUT nothing for the last reviewer.

And just like before I had to copy the whole method inside my own application _Item class and simply change the $joinLastCommenter variable/code to use the last reviewer column instead.

 

	/**
	 * Get items with permission check
	 *
	 * @param	array		$where				Where clause
	 * @param	string		$order				MySQL ORDER BY clause (NULL to order by date)

	[...]

	 * @param	bool		$joinLastCommenter	If true, will join the members table for the last commenter
	 * @param	bool		$showMovedLinks		If true, moved item links are included in the results
	 * @return	\IPS\Patterns\ActiveRecordIterator|int
	 */

 

Since I couldn't hook/extend it in any way, after copying the whole method as-is I updated 2 pieces of code to use the last review column instead of last comment:

			if( $joinLastCommenter and isset( static::$databaseColumnMap['last_review_by'] ) )
			{
				$selectClause .= ', last_commenter.*';
			}

		if ( $joinLastCommenter and isset( static::$databaseColumnMap['last_review_by'] ) )
		{
			$lastCommeneterColumn = static::$databaseColumnMap['last_review_by'];
			$select->join( array( 'core_members', 'last_commenter' ), array( 'last_commenter.member_id = ' . static::$databaseTable . '.' . static::$databasePrefix . $lastCommeneterColumn ) );
		}

 

P.S.: in the last codebox above the variable is wrongly named $lastCommenEterColumn.

[teraßyte]

Here's one more: in comments if you are a guest but can post as a normal member a message appears about registering an account or logging in. For reviews nothing appears, this is again another inconsistency.

[teraßyte]

On 22/9/2016 at 3:55 PM, teraßyte said:

Here's one more: in comments if you are a guest but can post as a normal member a message appears about registering an account or logging in. For reviews nothing appears, this is again another inconsistency.

Btw, I forgot to mention that after looking into this it turned out that there were several issues with the reviews.phtml templates included in the suite for all 5 applications (calendar, cms, downloads, gallery, nexus). I have made a bug report/ticket about it, ID #963988.

 

The ticket ID is just a reference for myself since no one else can see it, it has not been re-posted in tracker under the organized category.

[teraßyte]

And here we go with another one: there is core Queue extension to recount comments and resync the last one, but there is no Queue extension to do the same for reviews.

I am talking about the core Queue extension RebuildItemCounts, if you could also include 2 quick lines to recount and resync reviews it would be extremely nice to have. Right now I made a similar Queue extension for my application to rebuild reviews, same exact code but I simply changed those 2 lines inside the foreach cycle in the run method.

[Daniel F]

3 hours ago, teraßyte said:

And here we go with another one: there is core Queue extension to recount comments and resync the last one, but there is no Queue extension to do the same for reviews.

I am talking about the core Queue extension RebuildItemCounts, if you could also include 2 quick lines to recount and resync reviews it would be extremely nice to have. Right now I made a similar Queue extension for my application to rebuild reviews, same exact code but I simply changed those 2 lines inside the foreach cycle in the run method.

I have fixed this 2 days ago:) 

[teraßyte]

8 hours ago, Daniel F said:

I have fixed this 2 days ago:) 

1 less difference to worry about then

There are still plenty mentioned above though

[teraßyte]

Let's add another difference to this topic: the core Queue Extension RebuildContainerCounts does not resync the last review field for the container either. Just like the RebuildItemCounts one I mentioned in the replies just above (which Daniel says he's already fixed).

 

The queue contains this code:

			foreach( $iterator as $item )
			{
				$itemClass = $item::$contentItemClass;
				
				/* Deleting Pages categories or databases while a rebuild task remains can make Pages SPL autoloader fail */
				if ( ! class_exists( get_class( $item ) ) or ! class_exists( $itemClass ) )
				{
					throw new \IPS\Task\Queue\OutOfRangeException;
				}
				
				$item->resetCommentCounts();
				$item->setLastComment();
				$item->save();
				
				$last = $item->$idColumn;
			}

There is a double problem in the code above:

1. resetCommentCounts(): takes care ONLY of the items and comments counts without updating reviews count (even thought the code that have support for it in a couple of places). You should probably rename this function to resetContainerCounts() too, the current naming is confusing.
2. setLastComment(): obviously this one rebuilds only the last comment data, you need to add a call also to setLastReview().

 

For now I have extended again the resetCommentCounts() on my side in order to rebuild the reviews count too. As for setLastReview() I have simply made a duplicate of the whole Queue extension in my own application to add it.

[teraßyte]

After all these posts above let me say that this situation is completely ridiculous, I have to keep extending or almost copy/pasting entire files/classes/methods in order to work around on how Reviews are sloppily implemented. Yes, sloppily!

Nodes/Items/Comments are properly implemented everywhere, but it looks like Reviews were an afterthought that got the short end of the stick when you added them in the code. Just looking at my previous replies to this topic you can see how many issues there are (from small to big), I have also reported several other things as bugs which are not listed here. And I know I forgot to report other places as well, the amount of issues is even bigger

 

I doubt you're going to make any big changes at this point, but if you could fix 1-2 spots each minor release and properly standardize how things work for 4.2 it would be a great jump ahead in the right direction for the usability of the framework.

[teraßyte]

15 hours ago, teraßyte said:

Let's add another difference to this topic: the core Queue Extension RebuildContainerCounts does not resync the last review field for the container either. Just like the RebuildItemCounts one I mentioned in the replies just above (which Daniel says he's already fixed).

 

The queue contains this code:

			foreach( $iterator as $item )
			{
				$itemClass = $item::$contentItemClass;
				
				/* Deleting Pages categories or databases while a rebuild task remains can make Pages SPL autoloader fail */
				if ( ! class_exists( get_class( $item ) ) or ! class_exists( $itemClass ) )
				{
					throw new \IPS\Task\Queue\OutOfRangeException;
				}
				
				$item->resetCommentCounts();
				$item->setLastComment();
				$item->save();
				
				$last = $item->$idColumn;
			}

There is a double problem in the code above:

1. resetCommentCounts(): takes care ONLY of the items and comments counts without updating reviews count (even thought the code that have support for it in a couple of places). You should probably rename this function to resetContainerCounts() too, the current naming is confusing.
2. setLastComment(): obviously this one rebuilds only the last comment data, you need to add a call also to setLastReview().

 

For now I have extended again the resetCommentCounts() on my side in order to rebuild the reviews count too. As for setLastReview() I have simply made a duplicate of the whole Queue extension in my own application to add it.

In the end I couldn't simply extend the class and add in the reviews count. The resetCommentCounts() method doesn't check at all if an Item class has a Comment class, but assumes it always has one. You check for $nodeItemClass and return false, but you don't check for $commentClass.

I ended up copy/pasting the whole function and replaced the comments variables in it with reviews ones, I also renamed it and made my own Queue extension to call the new method and only setLastReview().

 

Usually I try to always extend existing methods without making a whole copy so if you change/fix those methods I get the changes/fixes almost always automatically, but sadly I can't always go that way like in this case

[bfarber]

I addressed all the issues with reviews you already raised, which were carried over to the Tracker (you can see which reports are marked fixed). If you find additional issues, continue to raise them in tickets and we will address them. Thanks!

It's important to keep in mind that just because reviews are different from comments, does not mean that they are an afterthought. Reviews are less used, and only supported in certain applications, and by their nature work slightly differently. If we have never had a need to "fix" something then the code won't be there for it - it's only logical.

 

[Adriano Faria]

On 03/05/2016 at 0:23 PM, teraßyte said:

Here's another difference between comments and reviews, the comments per page count is a method that returns a number while the reviews per page count is a static variable:

	/**
	 * @brief	[Content\Item]	Number of reviews to show per page
	 */
	public static $reviewsPerPage = 25;

	/**
	 * Get number of comments to show per page
	 *
	 * @return int
	 */
	public static function getCommentsPerPage()
	{
		return 25;
	}

 

Can you make things more consistent in the future please? 

 

On 03/10/2016 at 2:43 PM, bfarber said:

I addressed all the issues with reviews you already raised

@bfarber, how does it work? Number of reviews per page still appears as static variable on PS\Content\Item.

Is it already there on 4.1.16?

[bfarber]

There were several bug reports related to reviews that were addressed. That is not a bug and subsequently was not addressed via a bug report.

[Adriano Faria]

1 minute ago, bfarber said:

There were several bug reports related to reviews that were addressed. That is not a bug and subsequently was not addressed via a bug report.

So how can I set my own number of reviews per page?

[Martin A.]

50 minutes ago, Adriano Faria said:

So how can I set my own number of reviews per page?

Define $reviewsPerPage and/or getCommentsPerPage() in your item class.

[Adriano Faria]

1 hour ago, Martin A. said:

Define $reviewsPerPage

I did, using a setting, not a fixed number. The app do not work. Shows an error.

[Adriano Faria]

14 hours ago, Martin A. said:

Define $reviewsPerPage and/or getCommentsPerPage() in your item class.

 

13 hours ago, Adriano Faria said:

I did, using a setting, not a fixed number. The app do not work. Shows an error.

Let it fixed to a number, works:

public static $reviewsPerPage = 25;

The idea is to tie it to a settting:

	/**
	 * @brief	[Content\Item]	Number of reviews to show per page
	 */
	public static $reviewsPerPage = \IPS\Settings::i()->quizzes_display_reviews;

The error:

Quote

Whoops\Exception\ErrorException thrown with message "Constant expression contains invalid operations"

Stacktrace:
#6 Whoops\Exception\ErrorException in C:\wamp64\www\test\applications\quizzes\sources\Quiz\Quiz.php:813
#5 IPS\IPS:autoloader in C:\wamp64\www\test\system\Dispatcher\Front.php:270
#4 spl_autoload_call in C:\wamp64\www\test\system\Dispatcher\Front.php:270
#3 is_subclass_of in C:\wamp64\www\test\system\Dispatcher\Front.php:270
#2 IPS\Dispatcher\_Front:init in C:\wamp64\www\test\system\Dispatcher\Dispatcher.php:86
#1 IPS\_Dispatcher:i in C:\wamp64\www\test\index.php:13
#0 {main} in C:\wamp64\www\test\index.php:0

 

 

[newbie LAC]

13 hours ago, Adriano Faria said:

I did, using a setting, not a fixed number.

You can't use objects.

You can put your code in constructor or in setDefaultValues method.

[Adriano Faria]

1 minute ago, newbie LAC said:

You can put your code in constructor or in setDefaultValues method.

Sorry, can you show me an example?

[newbie LAC]

	/**
	 * Set Default Values
	 *
	 * @return	void
	 */
	public function setDefaultValues()
	{
		static::$reviewsPerPage = \IPS\Settings::i()->quizzes_display_reviews;
	}

 

[Adriano Faria]

On my controller? Model ?