APIv4 - Add SortableEntity type which auto-adjusts weights #22137
Conversation
|
(Standard links)
|
colemanw
force-pushed
the
api4SortableEntity
branch
3 times, most recently
from
05a6681
to
e9b96ae
Compare
| $existing = self::query('SELECT', $daoName, $fieldValues, "id", "$weightField = $newWeight"); | ||
| // Nothing to do if no existing record has this weight | ||
| if (empty($existing->N)) { | ||
| return $newWeight; |
There was a problem hiding this comment.
I'm not 100% sure about this because there's a couple places where the comments indicate it's supposed to create a gap for the new row, and there are several calls to this function. But logically the gap would already be there if it doesn't exist. I tried clicking around a couple places and it seems ok.
I'm not sure how to evaluate the api/search kit changes since I'm not sure where they're used. So have given merge-ready.
There was a problem hiding this comment.
@demeritcowboy I think it was an oversight. Prior to this PR there were no unit tests for CRM_Utils_Weight.
demeritcowboy
added
the
merge ready
colemanw
force-pushed
the
api4SortableEntity
branch
from
e9b96ae
to
02ea6e1
Compare
|
@demeritcowboy thanks for the review. I've just rebased this against master and added a few more unit tests + fixes to make sure |
|
Ha ha sneaky - a lot more changes now. Ok will take a look. |
|
Yea, I keep adding unit tests and it surfaces subtle bugs. It all came together in the |
This entity type will manage weight columns automatically, allowing items to be re-ordered easily. Simply by updating the weight of one record, others will auto-adjust to make room for it.
…u entity Excludes 'weight' from managed entity calculations for references, adds unit tests for the interaction of managed entities and sortable entities
colemanw
force-pushed
the
api4SortableEntity
branch
from
4289a83
to
2bf2436
Compare
|
I clicked around a bit and I think this is ok, just there's a lot going on in the api4 side that I don't have a good sense of what it affects, so wouldn't mind someone just doing a quick once-over. I notice searchkit for option values where you add a where for option_group_id returns nothing, but it's not from this PR. |
| */ | ||
| public static function enableAllComponents() { | ||
| $allComponents = array_keys(CRM_Core_Component::getComponents()); | ||
| if (Civi::settings()->get('enable_components') != $allComponents) { |
There was a problem hiding this comment.
Can we use !== here
There was a problem hiding this comment.
Here we are checking that both arrays contain the same elements, but they don't have to be in the same order.
There was a problem hiding this comment.
Hmm, interesting:
php -r '$a = array(1,2); $b = array(2,1); var_dump($a == $b); var_dump($a === $b);'
bool(false)
bool(false)
(php 7.4 at least)
There was a problem hiding this comment.
I recall stepping through this in the debugger and confirming that the condition is true the first pass but false subsequent test iterations, but wasn't looking too closely.
| @@ -21,13 +21,23 @@ class CRM_Core_OptionGroup { | |||
| /** | |||
| * $_domainIDGroups array maintains the list of option groups for whom | |||
| * domainID is to be considered. | |||
| * | |||
| * FIXME: Hardcoded list = bad. It would be better to make this a column in the civicrm_option_group table | |||
There was a problem hiding this comment.
I've always wondered about declaring our option groups in files (like with settings). It would take some work to do but I think it would be easier to maintain than the current way we manage the installation & could be hookable etc
|
Ouch - yeah - it made my head hurt a bit too - but I didn't spot anything of concern |
|
Thanks @eileenmcnaughton and @demeritcowboy! |
Overview
Gives APIv4 the ability to manage the "weight" of certain entities.
Before
After
Technical Details
This creates a trait for a new type: "SortableEntity", as well as a new
@orderByannotation which specifies the name of the column containing sorting data.