Quantity Rule is having wrong operator in its rule
'operator' => '<', which should be 'operator' => '>',

Last Order rule uses wrong order status selection

Condition to check if order is created less than 5 minutes uses wrong value,
I.e it uses minutes function gives the value in minutes not in timestamp,
It should use already provided function in the module commerce_fraud_timestamp_from_minutes

CommentFileSizeAuthor
#20 interdiff_14-21.txt2.51 KBvishalghyv
#20 commerce_fraud-slight_fixes_in_rules-3135019-21.patch4.65 KBvishalghyv
#14 commerce_fraud-slight_fixes_in_rules_3135019_14.patch2.72 KBvishalghyv
#11 SlightFixes_3135019_11.patch2.52 KBvishalghyv
#9 FixingAndNewRules-3135019-9.patch12.19 KBvishalghyv
#7 Screenshot (243).png112.81 KBvishalghyv
Screenshot (205).png72.99 KBvishalghyv
Screenshot (204).png73.65 KBvishalghyv
Screenshot (201).png76.44 KBvishalghyv

Comments

Vishalghyv created an issue. See original summary.

vishalghyv’s picture

vishalghyv commented

Firing rules after the event of saving order doesn't seem to be right for now.
Please excuse if anything is wrong.

vishalghyv’s picture

vishalghyv commented

And there is a change required in line 38 of the file commerce_fraud.rules_default.inc
For the rule Increase fraud score if order has x amount of products.

'operator' => '<', Should be changed to

'operator' => '>',

It worked in my system

vishalghyv’s picture

vishalghyv commented

po box rule is not firing even when the condition is negated, which doesn't make sense as if its not false should become true,
Maybe I am missing something

vishalghyv’s picture

vishalghyv commented

https://www.drupal.org/project/commerce/issues/1976480
Could be a issue to help resolve po box problem (Also a self reminder)

vishalghyv’s picture

vishalghyv commented

Have added the patch for po box in issue https://www.drupal.org/project/commerce_fraud/issues/2977626

vishalghyv’s picture

vishalghyv commented
StatusFileSize
new Screenshot (243).png112.81 KB

Wrong Data Selector for commerce_fraud_last_order rule, Although using direct input option in rules does fix it

vishalghyv’s picture

vishalghyv commented

Also the condition to select completed orders should check for commerce_fraud_timestamp_from_minutes in the above rule

vishalghyv’s picture

vishalghyv commented
Status: Active » Needs review
StatusFileSize
new FixingAndNewRules-3135019-9.patch12.19 KB
tr’s picture

tr commented

I triggered a test of your patch.

Note that if you want to reference other issues like https://www.drupal.org/project/commerce/issues/1976480 you can use this syntax: [ #1976480] (I had to add a space before the # so this would display properly, but you should remove that)

vishalghyv’s picture

vishalghyv commented
StatusFileSize
new SlightFixes_3135019_11.patch2.52 KB
vishalghyv’s picture

vishalghyv commented

In this patch problems mentioned in #3 , #7 and #8 are fixed. Is there need to create different patches for them?
Or any changes....
#3 problems is fixed
#7 I have removed the array to select the order statuses if it is required then will find a way to bring it back
#8 Is working in my system but if any other thing is required I will fix it.

vishalghyv’s picture

vishalghyv commented
Status: Needs review » Needs work
  • Last minute order rule have to be improved
vishalghyv’s picture

vishalghyv commented
Status: Needs work » Needs review
StatusFileSize
new commerce_fraud-slight_fixes_in_rules_3135019_14.patch2.72 KB
  • Corrected Quantity rule
  • Correct query in Last minute order rule
  • Correct Last minute order selection parameters
derekcresswell’s picture

derekcresswell commented
Status: Needs review » Reviewed & tested by the community

The changes here look ok to me. Though I must ask, is this achieving everything set out in the issue summary or do we need to create different issues.

In the summary :

Firstly, the rules are by default not enabled even when the module is installed, Since the rules are very strong so maybe it should be mentioned in the docs to go in workflow rules and enable them

This has not been done in the current patch. Is this going to be put in here or do we need to create a seperate patch / issue for this.

Marking as RTBC because it seems good in it's current state.

vishalghyv’s picture

vishalghyv commented

Thanks for the review,
About the documentation maybe after few things are committed probably be better,
But If it is required then I don't have any problem will add the patch.
And regarding PO box it is in #2781513: PO Box Regex

derekcresswell’s picture

derekcresswell commented

Related issues should be marked under the "Issue summary & relationships" section.

I think that something like that could warrant a separate issue. We just don't want one issue to deal with multiple problems at once (usually).

vishalghyv’s picture

vishalghyv commented
Issue summary: View changes
Related issues: +#2781513: PO Box Regex

Surely will add it in different issue.
Added relating issue

joshmiller’s picture

joshmiller
he/him
Primary language English
Location Indianapolis, Indiana, USA
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs issue summary update

Needs tests and an issue summary rewrite. Please rewrite the summary and title of this issue to address the issues your patch is fixing. Also, please write tests that show these things are broken and need fixing.

vishalghyv’s picture

vishalghyv commented
Title: Some likely problem in Rules » Fixes required in Quantity and Last Order Rule
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new commerce_fraud-slight_fixes_in_rules-3135019-21.patch4.65 KB
new interdiff_14-21.txt2.51 KB
  • Added the tests for both rule
  • Changed Issue title and status

  • joshmiller committed f0398f5 on 7.x-1.x authored by Vishalghyv 
    Issue #3135019 by Vishalghyv, DerekCresswell, TR, joshmiller: Fixes...
joshmiller’s picture

joshmiller
he/him
Primary language English
Location Indianapolis, Indiana, USA
commented
Status: Needs review » Fixed

Thanks @Vishalghyv :)

Status: Fixed » Closed (fixed)

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