Closed (fixed)
Project:
Mollom
Version:
6.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
7 Nov 2012 at 20:54 UTC
Updated:
24 Apr 2014 at 17:13 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
jazzslider commentedI've attached a patch that should take care of the basic use case here, though I have not yet had time to run any tests on it; I'd appreciate any community review!
Thanks,
Adam
Comment #3
jazzslider commentedHmm —that's a lot of test failures, but on reviewing them I'm not sure they have anything at all to do with the code I added. It seems as though the Drupal testing bot isn't able to reach the Mollom testing server, and that's causing a lot of processes to fail unexpectedly.
I'm putting this back into "needs review," because I think the automated tests will always fail based on the above observations —so it needs manual tests, then :)
Thanks!
Adam
Comment #4
jazzslider commentedInterestingly, I'm getting the same Internal Server Errors when I attempt to test this locally (with both "Plugin Developer Mode" enabled at mollom.com, and "test mode" enabled in the module itself). Perhaps there's an issue with dev.mollom.com at the moment?
Thanks!
Adam
Comment #5
sunThat's odd — I've informed the Mollom backend engineers about those errors.
Will review and comment about the proposed change shortly.
Comment #6
jazzslider commented#1: mollom-allow_unsure-1834860-1.patch queued for re-testing.
Comment #8
jazzslider commentedHello!
Looks like I patched against the wrong version (7.x-2.3 instead of 7.x-2.x, the development branch). I've updated the patch so that it should apply against 7.x-2.x, and also changed the verbiage from "allow" to "accept" per the comment I found in the code about adding these new actions.
Thanks!
Adam
Comment #9
jazzslider commentedWhoops, forgot to change to "needs review"; fixing.
Comment #10
jazzslider commentedOK, looks like we passed the automated tests. I've also tested this out manually on my local machine, and verified that it implements the requested behavior correctly. Can anyone else confirm?
Thanks!
Adam
Comment #11
sunSorry for not replying earlier.
So, hm. Originally, this behavior was rather foreseen to be toggled through the native "unsure"/binary mode of the Mollom API. The native unsure/binary mode essentially disables the possibility for a "unsure" response in the first place, which in turn means that posts are literally treated as either ham or spam, because only these two classification results are possible.
As originally mentioned in #1817446: Allow to retain and moderate unsure posts instead of showing CAPTCHA, the precursor for the new unsure setting was actually that issue, #1096412: Allow to disable fall-back CAPTCHA for text analysis.
So the essential difference is that with the unsure/binary mode, the Drupal client/module never receives a "unsure" response in the first place, which in turn means that all of the code paths for "unsure" responses inherently cannot be reached either. At the same time, the classification result is always ham or spam, but never unsure, so one wouldn't be able to see which posts were classified as unsure after the fact. Off-hand, I cannot see a reason for why anyone would want to see that, but OTOH, it might be useful.
Comment #12
jazzslider commentedHello!
I think either solution would satisfy my customer's use case; however, I think that following the options added in #1817446, the approach taken here would be a more predictable UX —if users are already looking at a "When text analysis is unsure" field, I think that's where users would first think to look for this.
Regarding the unsure/binary mode —that sounds like a viable approach too. However, I do think it's useful for site owners to know how Mollom makes its decisions, and being able to see whether something was "unsure" can help along those lines. One of the most common criticisms I've heard about any spam protection service I've used is that it's not easy to see how and why certain posts got rejected —transparency about that is very helpful in creating trust in the service.
Either way, though —I'd definitely like to see this capability incorporated into the module by one means or another; thanks for your help!
Comment #13
sunAlright. Attached patch combines this patch with the original #1096412: Allow to disable fall-back CAPTCHA for text analysis.
Comment #15
sunHm. The new configuration option is not presented very well yet:
1) IMHO, the new option is the least favorable, so it should come last.
2) We might want to clarify that the actual action is more fine-grained and can also mean that a post is discarded, if it's more "spammy" than "hammy". Although I'm not sure whether that's necessarily needed/noteworthy.
Comment #16
sunPlease ignore the test failures for now.
Only moved the option down:
I think this is acceptable and ready to go.
Comment #18
sun#16: mollom.binary.16.patch queued for re-testing.
Comment #19
sunThanks for reporting, reviewing, and testing! Committed to 7.x-2.x.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
Comment #21
sunComment #22
sunComment #24
sun#22: 0001-1834860-by-jazzslider-sun-Allow-to-accept-unsure-pos.patch queued for re-testing.
Comment #25
sunCommitted and pushed to 6.x-2.x.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.