FIXED: Updated blog management widget to work, including fixed translations #47

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@tractorcow
Collaborator

FIXED: Incorrect sprintf format string
FIXED: Incorrect reference to old page comment database structure. Updated to be compatible with new Comment module code.
REMOVED: Obsolete reference to comment admin CMS urls. Moderated and unmoderated comments don't seem to have distinct urls in the CMS anymore.

@tractorcow tractorcow FIXED: Incorrect sprintf format string
FIXED: Incorrect reference to old page comment database structure. Updated to be compatible with new Comment module code.
REMOVED: Obsolete reference to comment admin CMS urls. Moderated and unmoderated comments don't seem to have distinct urls in the CMS anymore.
f9620de
@halkyon halkyon commented on an outdated diff Aug 22, 2012
code/widgets/BlogManagementWidget.php
function CommentText() {
- if(!class_exists('Comment')) return false;
- $unmoderatedcount = DB::query("SELECT COUNT(*) FROM \"PageComment\" WHERE \"NeedsModeration\"=1")->value();
+ if(!class_exists('Comment'))
@halkyon
halkyon Aug 22, 2012 SilverStripe Ltd. member

You should really use braces on if blocks like this, in accordance to coding conventions here: http://doc.silverstripe.org/framework/en/trunk/misc/coding-conventions

@halkyon halkyon commented on an outdated diff Aug 22, 2012
code/widgets/BlogManagementWidget.php
@@ -35,14 +37,10 @@ function CommentText() {
function CommentLink() {
- if(!Permission::check('BLOGMANAGEMENT') || !class_exists('Comment')) return false;
- $unmoderatedcount = DB::query("SELECT COUNT(*) FROM \"PageComment\" WHERE \"NeedsModeration\"=1")->value();
-
- if($unmoderatedcount > 0) {
- return "admin/comments/unmoderated";
- } else {
- return "admin/comments";
- }
+ if(!Permission::check('BLOGMANAGEMENT') || !class_exists('Comment'))
@halkyon
halkyon Aug 22, 2012 SilverStripe Ltd. member

Same as my previous comment.

@tractorcow
Collaborator

Excellent, I actually didn't realise I had made an error here.

Thanks for the clarification. I'll make sure to follow this in future.

@cbarberis cbarberis closed this Feb 28, 2013
@tractorcow
Collaborator

Hi Carlos, do you mind if I ask for some feedback on this pull request please?

I realise that the module incompatibility has been since resolved, but there's still remaining a few logical issues:

  • sprintf("%i") might work on some systems, but %d is the correctly documented integer placeholder.
  • NeedsModeration is the logical inverse to Moderated; The NeedsModeration = 1 condition needs to be changed to Moderated = 0
  • admin/comments/unmoderated doesn't work in Silverstripe 3.0 (at least, not at the time of testing, but may have since been fixed). That's why that code was pulled out.

Do you mind if I put these into a separate pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment