Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REACH-updating credits #9060

Merged
merged 6 commits into from
Dec 25, 2019
Merged

Conversation

gotlieb
Copy link
Contributor

@gotlieb gotlieb commented Dec 24, 2019

@@ -52,4 +52,10 @@ public function hasObjectChanged($sourceObject)
{
return false;
}

public function validateForUpdate($sourceObject, $propertiesToSkip = array())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed?

{
return false;
}
return get_class($object) == 'kReoccurringVendorCredit';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String matching only with ===

if (isset($this->fromDate) && isset($this->toDate))
{
if ($this->fromDate > $this->toDate)
throw new KalturaAPIException(KalturaReachErrors::INVALID_CREDIT_DATES, $this->fromDate, $this->toDate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brackets

*/
public function validateForUpdate($sourceObject, $propertiesToSkip = array())
{
if (!isMatchingCoreClass($sourceObject))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This code cannot be called;
  2. The function holds several case , break it to smaller cases.

/* @var $sourceObject kTimeRangeVendorCredit */
if($this->toDate && $this->toDate != $sourceObject->getToDate())
if ( method_exists($sourceObject,'getToDate') && $this->fromDate > $sourceObject->getToDate())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be used based on object functions , method_exists is bad practice.

@@ -39,7 +39,7 @@ public function getMapBetweenObjects()
public function validateForInsert($propertiesToSkip = array())
{
$this->validatePropertyNotNull("fromDate");
parent::validateForInsert(array("credit"));
parent::validateForInsert(array_merge($propertiesToSkip,array("credit")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String without processing - signle qoutes

return parent::toObject($dbObject, $propsToSkip);
}

public function hasObjectChanged($sourceObject)
{
if(parent::hasObjectChanged($sourceObject))
if (parent::hasObjectChanged($sourceObject))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brackets

{
return false;
}
return get_class($object) == 'kUnlimitedVendorCredit';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string matching

{
return false;
}
return get_class($object) == 'kVendorCredit';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string matching

@gotlieb gotlieb merged commit 5d58f76 into Orion-15.14.0 Dec 25, 2019
@MosheMaorKaltura
Copy link
Contributor

reveiwed

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

Successfully merging this pull request may close these issues.

None yet

2 participants