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

[REF] Update Product Create to use hooks and also switch the manage p… #20822

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

seamuslee001
Copy link
Contributor

…roduct/premium page to use APIv4 Product Entity

Overview

This updates the Product create BAO to trigger pre/post hooks and also to update the Manage Product forms to use the newly added v4 API entity

Before

No hooks triggered and DAO used

After

Hooks triggered and APIv4 Entity used

ping @colemanw @demeritcowboy @eileenmcnaughton @totten

@civibot
Copy link

civibot bot commented Jul 9, 2021

(Standard links)

@civibot civibot bot added the master label Jul 9, 2021
@@ -291,10 +292,16 @@ public function postProcess() {
$this->_processImages($params);

// Save the premium product to database
$premium = CRM_Contribute_BAO_Product::create($params);
// If we're updating, we need to pass in the premium product Id
Copy link
Member

Choose a reason for hiding this comment

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

Actually you don't. APIv4::update doesn't require a where param if $values contains an id :)

Comment on lines 296 to 301
if ($this->_action & CRM_Core_Action::UPDATE) {
$premium = Product::update(FALSE)->setValues($params)->addWhere('id', '=', $this->_id)->execute()->first();
}
else {
$premium = Product::create(FALSE)->setValues($params)->execute()->first();
}
Copy link
Member

Choose a reason for hiding this comment

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

Or you could just turn these 6 lines into a 1-liner by using

Suggested change
if ($this->_action & CRM_Core_Action::UPDATE) {
$premium = Product::update(FALSE)->setValues($params)->addWhere('id', '=', $this->_id)->execute()->first();
}
else {
$premium = Product::create(FALSE)->setValues($params)->execute()->first();
}
$premium = Product::save(FALSE)->addRecord($params)->execute()->first();

@seamuslee001
Copy link
Contributor Author

Thanks @colemanw I have committed your suggestion now

@seamuslee001 seamuslee001 force-pushed the use_product_api_form_hook branch from a9cee0d to 5edd001 Compare July 12, 2021 02:36
@eileenmcnaughton
Copy link
Contributor

@colemanw is this all good now?

@@ -35,8 +37,7 @@ public function getDefaultEntity() {
public function setDefaultValues() {
$defaults = parent::setDefaultValues();
if ($this->_id) {
$params = ['id' => $this->_id];
CRM_Contribute_BAO_Product::retrieve($params, $tempDefaults);
$tempDefaults = Product::get(FALSE)->addWhere('id', '=', $this->_id)->execute()->first();
Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 I'm just looking at you putting check permissions to FALSE. This is one of those places where ideally we WOULD have permissions kick in - ie this seems to be the form to configure products & hence the ability to get & save products should be material. I can think of 2 reasons why you put FALSE here

  1. habit
  2. our permissions for product are too tight & it failed without bypassing them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok @eileenmcnaughton I have changed this to not override check permissions now and have updated the entity permissions so that product matches contribution which seems to be the most accurate thing here IMO

…roduct/premium page to use APIv4 Product Entity

Update to use suggestion from Coleman

Set Product permissions to be the same as contribution entity
@seamuslee001 seamuslee001 force-pushed the use_product_api_form_hook branch from 5edd001 to 7e49aa9 Compare September 6, 2021 23:07
@@ -90,7 +91,7 @@ public static function create($params) {
];
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a pre-existing condition and not a blocker for this PR, but I really hate this array of defaults. They should be set in the schema. Also, is_active should default to TRUE for consistency with every other entity.

@colemanw
Copy link
Member

colemanw commented Sep 6, 2021

I'm OK with merging this as-is, but I'd push for an additional cleanup of deprecating this create function, gutting it down to a single line calling self::writeRecord() and using a pre hook to perform the url simplification and setting default currency.

@colemanw colemanw merged commit 0bbc704 into civicrm:master Sep 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the use_product_api_form_hook branch September 7, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants