[bug] a canceled order may payed again and not show as dingads #2

milkliker · 2012-09-01T07:05:27Z

eg. the adid of the ad is "1", and the first order last from 2012-01-01 to 2012-12-31,
when he refund at 2012-08-01, we create a negative order.
but when it goes to payed again on 2012-09-01, so there will be 2 payed order now, and one in the neg array.

and array_diff will ignore the double existence, and return only ad 2, 3 without 1.

$dingAds = array(1, 2, 3, 4, 1);
$negAds = array(4, 1);

$ar = array_diff($dingAds, $negAds);
var_dump($ar);// array(2, 3);

so I do suggest not use negative order to fix the refund problem, cause when we got only the active service, it's can not tell whether it's on or not!

jianshuo · 2012-09-01T07:42:34Z

Very valid. The original algorithm was effectively saying when an adid was
cancelled, it is never be able to be dinged again.

We can fix it by replay the orders by time, but of there is other
solutions, it will be better.

Jian Shuo

在 2012-9-1，下午3:05，Zhao Jun [email protected] 写道：

eg. the adid of the ad is "1", and the first order last from 2012-01-01 to
2012-12-31,
when he refund at 2012-08-01, we create a negative order.
but when it goes to payed again on 2012-09-01, so there will be 2 payed
order now, and one in the neg array.

and array_diff will ignore the double existence, and return only ad 2, 3
without 1.

$dingAds = array(1, 2, 3, 4, 1);$negAds = array(4, 1);
$ar = array_diff($dingAds, $negAds);var_dump($ar);// array(2, 3);

so I do suggest not use negative order to fix the refund problem, cause
when we got only the active service, it's can not tell whether it's on or
not!

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/2.

jianshuo · 2012-09-01T08:01:26Z

or change it to use neg to cancel order- compare orderid to remove, not adid

Another known bug is, 0 price order cannot be cancelled this way. This will
also lead to the change of design.

In the current design, the table should be called Service instead of order.
It is closer to service than order.

I will go further on the service / order separation path by writing some
code and be back to the discussion. You should do the same - explore the
different path before finalize a design.

Jian Shuo

�$B:_�(B 2012-9-1�$B!$2<8a�(B3:42�$B!$�(BJian Shuo Wang [email protected] �$B<LF;!'�(B

Very valid. The original algorithm was effectively saying when an adid was
cancelled, it is never be able to be dinged again.

We can fix it by replay the orders by time, but of there is other
solutions, it will be better.

Jian Shuo

�$B:_�(B 2012-9-1�$B!$2<8a�(B3:05�$B!$�(BZhao Jun [email protected] �$B<LF;!'�(B

eg. the adid of the ad is "1", and the first order last from 2012-01-01 to
2012-12-31,
when he refund at 2012-08-01, we create a negative order.
but when it goes to payed again on 2012-09-01, so there will be 2 payed
order now, and one in the neg array.

and array_diff will ignore the double existence, and return only ad 2, 3
without 1.

$dingAds = array(1, 2, 3, 4, 1);$negAds = array(4, 1);
$ar = array_diff($dingAds, $negAds);var_dump($ar);// array(2, 3);

so I do suggest not use negative order to fix the refund problem, cause
when we got only the active service, it's can not tell whether it's on or
not!

�$B!=�(B
Reply to this email directly or view it on
GitHubhttps://github.com//issues/2.

milkliker · 2012-09-01T09:56:56Z

I've tried to write single "service" instead of service + order today, and found it's really effective.
Cause the responsibility of the " should be order" now is only for log the price data ( the listPrice should be logger in service).
so we can merge the price(real money) in service.

and what I found more is we should create a "valid date" of any service.

like if someone buy 50 refresh service, and we may make the valid days of one year.
so by the end of valid day, if there is still some quota remains, but we'll cancel the quota with charge for financially reason.

abstract class Service {
    protected $status, $price, $listPrice, $userId, $startTime, $endTime, $days;
    function purchase($params) {
        foreach ($this->allowfields() as $key => $required) {
            $this->$key = $params->$key;
            if ($required && is_null($this->$key)) throw new Exception('need required field:' . $key);
        }
        $this->save();
    }

    function allowfields() {
        return array('userId' => 1, 'days' => 1, 'listPrice' => 1);
    }

    function pay() {
        if ($this->status == 'payed') throw new Exception('can not pay again!');
        $time = time();
        $this->startTime = $time;
        $this->endTime = $time + $this->days * 86400;
        $this->status = 'payed';
        $this->price = $this->uc()->ratio * $this->listPrice;
        $this->uc()->out($this->listPrice);
        $this->save();
    }

    function uc() {
        return new UserAccount($this->userId);
    }

    function cancel() {

    }

    abstract function shippedPercentage($time);
}

abstract class DaybasedService extends Service {
    function shippedPercentage($time) {
        return ;
    }
}

class DingService extends DaybasedService {
    function allowfields() {
        return array('adId' => 1, 'category' => 1, 'area' => 1) + parent::allowfields();
    }
}

jianshuo · 2012-09-01T15:57:12Z

Some comments:

Well done. cleaner, but still need some work.
Minor things first: paid, not payed. Error message should be correct
English. Capitalize first letter of the sentence. etc...
Why Order have to be attached to a UserId - I am not saying no - I
didn't think about this yet. Need to dive deeper into business by asking
Yifeng and Huahua
Why a piece of code in Order class need to understand anything about
UserAccount's ratio? I believe it is UserAccount's responsibility to pay
order, not order's responsibility to charge userAccount (I need to write
code to justify this statement).
Pay time is the start time seems to be over simplified of business
requirement. For many of the renew, pay time is way ahead of starttime.
Need to polish function names.
Still need to justify the three fields - startTime, endTime, days. We
need only two. Even when we have three, we have to be absolutely clear
about which one is the buffer data, and which two are the solid data
source. In case there is conflicts, we can still rely on the basic two. For
example, we can claim days as buffer data. If that is the case, we should
never read days field in any systems outside order. (Getter should throw
exception to protect it).
8.

On Saturday, September 1, 2012, Zhao Jun wrote:

I've tried to write single "service" instead of service + order today, and
found it's really effective.
Cause the responsibility of the " should be order" now is only for log the
price data ( the listPrice should be logger in service).
so we can merge the price(real money) in service.

and what I found more is we should create a "valid date" of any service.

like if someone buy 50 refresh service, and we may make the valid days of
one year.
so by the end of valid day, if there is still some quota remains, but
we'll cancel the quota with charge for financially reason.

abstract class Service {
protected $status, $price, $listPrice, $userId, $startTime, $endTime, $days;
function purchase($params) {
foreach ($this->allowfields() as $key => $required) {
$this->$key = $params->$key;
if ($required && is_null($this->$key)) throw new Exception('need required field:' . $key);
}
$this->save();
}
function allowfields() {
    return array('userId' => 1, 'days' => 1, 'listPrice' => 1);
}

function pay() {
    if ($this->status == 'payed') throw new Exception('can not pay again!');
    $time = time();
    $this->startTime = $time;
    $this->endTime = $time + $this->days * 86400;
    $this->status = 'payed';
    $this->price = $this->uc()->ratio * $this->listPrice;
    $this->uc()->out($this->listPrice);
    $this->save();
}

function uc() {
    return new UserAccount($this->userId);
}

function cancel() {

}

abstract function shippedPercentage($time);}
abstract class DaybasedService extends Service {
function shippedPercentage($time) {
return ;
}}
class DingService extends DaybasedService {
function allowfields() {
return array('adId' => 1, 'category' => 1, 'area' => 1) + parent::allowfields();
}}

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-8211622.

Jian Shuo Wang
http://home.wangjianshuo.com | [email protected] | +8613916146826 |
Baixing.com

milkliker · 2012-09-02T01:32:32Z

It's service instead of order now, cause more likely.
My mistake, sorry.
Just want to declaim an ownership of service. If someone want to make a refund, we have to check the ownership, where the payment comes, there the refund goes.

Why Order have to be attached to a UserId.
I'm not sure either. I'll make clear by writing code again.

Why a piece of code in Order class need to understand anything about UserAccount's ratio.
Yes, you right! We do need a param named time of function pay.

For many of the renew, pay time is way ahead of starttime.
Yes, always need.

Need to polish function names.
I have verified by some code to find out: the days, or we say valid period, is need to make more meaningful.
Just like if you bought a pay first massage service, when you paid, you may got a ticket saying you can have one hour service, it's better than you got a ticket saying your time is start now, and end an hour later, but which is a fake timing.

Still need to justify the three fields - startTime, endTime, days.

milkliker · 2012-09-02T02:09:21Z

And one more thing. It's more effective to make one single service record to decide itself active or not.
Neg orders takes many other issues in the system, cause you always need to do additional work after searching.

// a single record can decide active status, because the canceled service is no longer status "paid"
class Ad {
    function isOnService($time) {
        $activeService = graph("{$this->id}/service?status=paid&startTime=[,{$time}]&endTime=[{$time},]");
        return count($activeService['data']) > 0;
    }
}

//if we use neg service when refund.
class Ad {
    function isOnService($time) {
        $activeService = graph("{$this->id}/service?status=paid&startTime=[,{$time}]&endTime=[{$time},]");
        $on = 0;
        foreach ($activeService['data'] as $s) {
            if ($s->price >= 0)
                $on++;
            else
                $on--;
        }
        return $on > 0;
    }
}

And many other places we all need to do like this.

jianshuo · 2012-09-02T05:21:46Z

I agree. Just need to find a solution that also works with Yifeng. You can
call him to figure out.

高奕峰186-2193-5860

Jian Shuo

在 2012-9-2，上午10:09，Zhao Jun [email protected] 写道：

And one more thing. It's more effective to make one single service record
to decide itself active or not.
Neg orders takes many other issues in the system, cause you always need to
do additional work after searching.

// a single record can decide active status, because the canceled
service is no longer status "paid"class Ad {
function isOnService($time) {
$activeService =
graph("{$this->id}/service?status=paid&startTime=[,{$time}]&endTime=[{$time},]");
return count($activeService['data']) > 0;
}}
//if we use neg service when refund.class Ad {
function isOnService($time) {
$activeService =
graph("{$this->id}/service?status=paid&startTime=[,{$time}]&endTime=[{$time},]");
$on = 0;
foreach ($activeService['data'] as $s) {
if ($s->price >= 0)
$on++;
else
$on--;
}
return $on > 0;
}}

And many other places we all need to do like this.

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/2#issuecomment-8218454.

jianshuo · 2012-09-02T05:21:46Z

On 7, yes. Days are first class data - most accurate. Start Time is also
first class data. If that is the case, we design endTime as buffer data and
need to recalculated from first two if needed.

Jian Shuo

在 2012-9-2，上午9:32，Zhao Jun [email protected] 写道：

It's service instead of order now, cause more likely.
My mistake, sorry. > 3. Why Order have to be attached to a UserId.
Just want to declaim an ownership of service. If someone want to make a
refund, we have to check the ownership, where the payment comes, there the
refund goes. > 4. Why a piece of code in Order class need to understand
anything about UserAccount's ratio. I'm not sure either. I'll make clear by
writing code again. > 5. For many of the renew, pay time is way ahead of
starttime. Yes, you right! We do need a param named time of function pay. >
Need to polish function names. Yes, always need. > 7. Still need to
justify the three fields - startTime, endTime, days. I have verified by
some code to find out: the days, or we say valid period, is need to make
more meaningful. Just like if you bought a pay first massage service, when
you paid, you may got a ticket saying you can have one hour service, it's
better than you got a ticket saying your time is start now, and end an hour
later, but which is a fake timing.

—
Reply to this email directly or view it on
GitHubhttps://github.com/[bug] a canceled order may payed again and not show as dingads #2#issuecomment-8218282.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] a canceled order may payed again and not show as dingads #2

[bug] a canceled order may payed again and not show as dingads #2

milkliker commented Sep 1, 2012

jianshuo commented Sep 1, 2012

jianshuo commented Sep 1, 2012

milkliker commented Sep 1, 2012

jianshuo commented Sep 1, 2012

milkliker commented Sep 2, 2012

milkliker commented Sep 2, 2012

jianshuo commented Sep 2, 2012

jianshuo commented Sep 2, 2012

[bug] a canceled order may payed again and not show as dingads #2

[bug] a canceled order may payed again and not show as dingads #2

Comments

milkliker commented Sep 1, 2012

jianshuo commented Sep 1, 2012

jianshuo commented Sep 1, 2012

milkliker commented Sep 1, 2012

jianshuo commented Sep 1, 2012

milkliker commented Sep 2, 2012

milkliker commented Sep 2, 2012

jianshuo commented Sep 2, 2012

jianshuo commented Sep 2, 2012