The blacklist can get very long, so we should split it by type/reason.

CommentFileSizeAuthor
#42 mollom-HEAD.blacklist.42.patch9.89 KBsun
#40 mollom-DRUPAL-6--1.blacklist.40.patch10.42 KBsun
#39 mollom-DRUPAL-6--1.blacklist.38.patch9.5 KBsun
#37 mollom-DRUPAL-6--1.blacklist.37.patch9.18 KBsun
#36 mollom-DRUPAL-6--1.blacklist.36.patch7.03 KBsun
#35 mollom-DRUPAL-6--1.blacklist.35.patch6.72 KBsun
#34 mollom-HEAD.blacklist.33.patch6.34 KBsun
#32 mollom-HEAD.blacklist.32.patch6.02 KBsun
#31 mollom-HEAD.blacklist.31.patch5.79 KBsun
#29 mollom-DRUPAL-6--1.blacklist.29.patch7.31 KBsun
#28 mollom-DRUPAL-6--1.blacklist.28.patch7.33 KBsun
#26 mollom-HEAD.blacklist.26.patch7.28 KBsun
#24 mollom-HEAD.blacklist.24.patch7.79 KBsun
#22 mollom-DRUPAL-6--1.blacklist.22.patch7.15 KBsun
#20 mollom-DRUPAL-6--1.blacklist.20.patch6.49 KBsun
#19 mollom-DRUPAL-6--1.blacklist.19.patch6.15 KBsun
#18 mollom-DRUPAL-6--1.blacklist.18.patch2.71 KBsun
#16 mollom-HEAD.blacklist-ux.16.patch9.52 KBsun
#14 mollom-HEAD.blacklist-ux.14.patch8.3 KBsun
#12 mollom-DRUPAL-6--1.blacklist-ux.12.patch8.13 KBsun
#11 mollom-DRUPAL-6--1.blacklist-ux.10.patch8.12 KBsun
#10 mollom-DRUPAL-6--1.blacklist-ux.9.patch8.93 KBsun
#9 mollom-DRUPAL-6--1.blacklist-ux.8.patch6.2 KBsun
#7 mollom-DRUPAL-6--1.blacklist-ux.7.patch3.08 KBsun
#6 mollom-DRUPAL-6--1.blacklist-ux.6.patch2.44 KBsun
#5 mollom-DRUPAL-6--1.blacklist-split.5.patch4.47 KBsun
#4 mollom-DRUPAL-6--1.blacklist-split.4.patch4.04 KBsun
#2 mollom-HEAD.blacklist-split.2.patch3.92 KBsun
#1 mollom-HEAD.blacklist-split.1.patch3.96 KBsun
mollom-HEAD.blacklist-split.0.patch3.32 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.blacklist-split.1.patch3.96 KB

- Make the current type be preselected as default for new entries.

- Redirect back to the originating type-specific blacklist page after deleting an entry.

Note: I'm currently unable to delete a profanity entry - shouldn't be caused by this patch though.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.blacklist-split.2.patch3.92 KB

Fixed phpDoc.

dries’s picture

dries commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-split.4.patch4.04 KB

Backport to D6.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-split.5.patch4.47 KB

Thanks for reporting, reviewing, and testing! Committed to D6.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Split blacklist by type/reason » Improve blacklist UX
Status: Fixed » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.6.patch2.44 KB

- Moved the add form to the top.

- Removed "Reason" column.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.7.patch3.08 KB

Attached patch additionally fixes the blacklist UI test accordingly.

dries’s picture

dries commented

0 zero passes in #7

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.8.patch6.2 KB

Added tests for the profanity blacklist, which reveal that blacklist entries are not deleted on the backend currently.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.9.patch8.93 KB

ok, it's a client-side bug. This patch fixes it.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.10.patch8.12 KB

Argh. Stale debugging code.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist-ux.12.patch8.13 KB

Tested manually and attached patch passes all tests.

dries’s picture

dries commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to DRUPAL-6. Needs to be ported to Drupal 7.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-HEAD.blacklist-ux.14.patch8.3 KB

Ported to HEAD.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.blacklist-ux.14.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.blacklist-ux.16.patch9.52 KB

This one should be better, I hope.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Improve blacklist UX » Blacklist entries not properly added and deleted
Version: 7.x-1.x-dev » 6.x-1.x-dev
Category: task » bug
Status: Fixed » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.18.patch2.71 KB

Don't want to clobber the queue with lots of closely related issues, so re-opening and slightly re-purposing this issue.

I just learned that we are not fully conforming to http://mollom.com/api/addBlacklistText, which perhaps explains why I'm not able to delete certain entries. The particular one I tried does indeed contain trailing white-space.

Aside from this patch, there's more to it: Mollom returns TRUE for mollom.removeBlacklistText, even if the value does not exist or when the operation was not successful. This needs to be improved on the backend, and on the client-side, we want to add tests that verify the expected results for all possible (and partially bogus) operations.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.19.patch6.15 KB

Attached patch fixes lots of outdated and bogus tests. Now I also understand why the blacklist of the testing keys always contains garbage.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.20.patch6.49 KB
dries’s picture

dries commented

1. 

+++ mollom.admin.inc	18 Sep 2010 17:14:20 -0000
@@ -387,13 +387,33 @@ function mollom_admin_blacklist_form(&$f
+    'text' => trim($form_state['values']['entry']['text'], ' '),

This trim is no longer necessary. The backend takes care of that. Probably means we can revert some of the code changes.

2. Now we splitted the blacklist in multiple blacklists, we might want to rename the menu from 'Blacklist' to 'Blacklists'?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.22.patch7.15 KB

Thanks! I didn't fully revert though, since I prefer to set the $data properties that are sent to Mollom more explicitly, and because both the add and the delete submit handlers are more consistent this way.

In a next step, we should think about further usability improvements for the blacklist. While working on mollom.com's rather large one, I had the following clues:

- First and foremost, we want to sort alphabetically by 'text', and then by 'context'. Ideally, this should happen on the backend already, but we can also do it on the client-side.

- Would be nice to have checkboxes to be able to delete multiple.

dries’s picture

dries commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to DRUPAL-6--1. Thanks sun.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-HEAD.blacklist.24.patch7.79 KB

Ported to HEAD.

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs review
StatusFileSize
new mollom-HEAD.blacklist.26.patch7.28 KB

Unfortunately, the previous patch broke our tests (attached patch won't be tested, since tests on Mollom HEAD are failing). In trying to make them pass again, I struggled again with the testing profile, but was finally able to make it work. That revealed a couple of other glitches along the way.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Patch (to be ported)

Thanks for reviewing. Committed to Mollom HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.28.patch7.33 KB

I'm going to run tests manually.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.29.patch7.31 KB

um. OK, so rather this one -- getting query strings right in D6 is a major pain; thankfully, we fixed that for D7!

Setting to RTBC, as I'm not sure whether you know a better way to build query strings in D6.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Blacklist entries not properly added and deleted » Blacklist entries cannot be properly added and deleted
Version: 6.x-1.x-dev » 7.x-1.x-dev
Category: bug » task
Status: Reviewed & tested by the community » Active

Committed #29 to D6 for now, so as to be able to continue with further improvements for D7.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new mollom-HEAD.blacklist.31.patch5.79 KB

+ Added "match" to the blacklist UI.

+ Added an instant filter to search existing entries. ROCKS. Try it out! :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.blacklist.32.patch6.02 KB

Alright. The blacklist on mollom.com is so large that I had to do some heavy performance optimizations. ;)

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.blacklist.32.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.blacklist.33.patch6.34 KB

Final edition. I think this is ready to fly. Can't wait to backport this + deploy on mollom.com. ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.35.patch6.72 KB

Backported to D6.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.36.patch7.03 KB

I don't think it can get faster than this. Let's try again. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.37.patch9.18 KB

Look, I didn't really care for the blacklist before. But hey, with this patch, I'd happily administer blacklists the entire day! 8)

dries’s picture

dries commented

I absolutely love it! Applied it on mollom.com for testing purpose.

I noticed there was a @todo in the patch, which is the only reason why I haven't committed it yet. ;-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.38.patch9.5 KB

So here's the final version with that remaining @todo resolved. Totally love it, too! :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.blacklist.40.patch10.42 KB

Manually ran tests + this one passes.

dries’s picture

dries commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to DRUPAL-6--1. Thanks sun.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-HEAD.blacklist.42.patch9.89 KB

Ported to HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Also committed mollom.admin.js to D6, which got lost. ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 8b64982 on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    #911328 by sun: Improved blacklist user interface.
  • Commit ab007df on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #911328 by sun: Blacklist entries cannot be properly added and deleted.
  • Commit d899cb1 on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #911328 by sun: blacklist entries not properly added and deleted...
  • Commit f2ca66b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #911328 by sun: split blacklist by type/reason.
  • Commit f967cb7 on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #911328 by sun: Fixed Blacklist tests broken.

  • Commit 8b64982 on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    #911328 by sun: Improved blacklist user interface.
  • Commit ab007df on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #911328 by sun: Blacklist entries cannot be properly added and deleted.
  • Commit d899cb1 on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #911328 by sun: blacklist entries not properly added and deleted...
  • Commit f2ca66b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #911328 by sun: split blacklist by type/reason.
  • Commit f967cb7 on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #911328 by sun: Fixed Blacklist tests broken.