CiviGrant - Cleanup managed entities - fixes dev/core#3161 #23140

colemanw · 2022-04-08T13:52:17Z

Overview

Improves the managed options that come with the CiviGrant extension, to prevent problems documented in dev/core#3161.

Technical Details

Ensure labels are translated
Set stricter cleanup policy for entities that should be removed on uninstall
Make grant_status options unmanaged. They will not be updated by the system ever, but they will be deleted on uninstall because the option group is a managed entity.
Use the 'match' option to prevent errors when items already exist (this also allows the 5.47 upgrader to be simplified).

- Ensure labels are translated - Set stricter cleanup policy for entities that should be removed on uninstall - Make grant_status options unmanaged. They will not be updated by the system ever, but they will be deleted on uninstall because the option group is a managed entity.

civibot · 2022-04-08T13:52:19Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

totten · 2022-04-12T03:42:25Z

@colemanw So I've had a mix of good+bad while testing this.

First, for setup, I prepared a DB snapshot with some CiviGrant data:

Rebuild on 5.45
Configure types
                Emergency (#1), enabled (unedited, default)
                Family Support (#2), enabled, renamed to "Familial Assistance"
                General Protection (#3), deleted (no edits)
                Impunity (#4), disabled
                Magic (#5), new+enabled, 
Configure statuses
                Submitted (#1)
                Eligible (#2)
                Ineligible (#3)
                Paid (#4)
                Awaiting Information (#5), enabled, renamed to "Limbo"
                Withdrawn (#6), disabled
                Approved for Payment (#7), deleted
Add some grant records
                #1, Alida Adams, Limbo, Magic, $1000, Rx 5-Apr-22
                #2, Brent Adams, Eligible, Familial Support, $500=>$475, Rx 7-Apr-22
Run "civibuild snapshot dmaster"

To test an upgrade, I used this process: https://gist.github.com/totten/1b2ab35c941aede8550f1627ac20699c. After each DB upgrade, I would view 1/civicrm/admin/options/grant_type?reset=1 and /civicrm/admin/options/grant_status?reset=1 to see the resulting configuration.

Here the results from various upgrade paths:

Upgrade Path	Grant Types	Grant Statuses
480	good	reset to defaults 🛑
48head	good	reset to defaults 🛑
48patch	good	good ✔️
474	good	reset to defaults 🛑
474_then_48head	good	reset to defaults 🛑
474_then_48patch	good	all empty 🛑

So if you're doing a direct upgrade (5.45=>5.48), then 48patch is definitely better than 48head. It preserves the data.

But if you're doing multiple steps (5.45=>5.47=>5.48), then 48patch is worse than 48head - because you end up losing all statuses. (I haven't looked at why.)

totten · 2022-04-12T07:37:55Z

I stepped through the upgrade for 5.45=>5.47=>5.48 and confirmed that the doCoreFinish() step (which flushes things and reconciles managed-entities) is where it looses all statuses:

Before that step, the DB data looks like:

mysql> select * from civicrm_managed where module='civigrant'; 

+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
| id | module    | name                                                                     | entity_type   | entity_id | cleanup | entity_modified_date |
+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
| 30 | civigrant | Navigation_CiviGrant                                                     | Navigation    |       213 | unused  | NULL                 |
| 32 | civigrant | Navigation_CiviGrant_Navigation_Grant_Status                             | Navigation    |       215 | unused  | NULL                 |
| 31 | civigrant | Navigation_CiviGrant_Navigation_Grant_Types                              | Navigation    |       214 | unused  | NULL                 |
| 26 | civigrant | Navigation_Grants                                                        | Navigation    |       104 | unused  | NULL                 |
| 27 | civigrant | Navigation_Grants_Navigation_Dashboard                                   | Navigation    |       105 | unused  | NULL                 |
| 29 | civigrant | Navigation_Grants_Navigation_Find_Grants                                 | Navigation    |       107 | unused  | NULL                 |
| 28 | civigrant | Navigation_Grants_Navigation_New_Grant                                   | Navigation    |       106 | unused  | NULL                 |
| 13 | civigrant | OptionGroup_advanced_search_options_OptionValue_CiviGrant                | OptionValue   |       165 | unused  | NULL                 |
| 14 | civigrant | OptionGroup_contact_view_options_OptionValue_CiviGrant                   | OptionValue   |       132 | unused  | NULL                 |
| 16 | civigrant | OptionGroup_grant_status                                                 | OptionGroup   |        23 | unused  | NULL                 |
| 33 | civigrant | OptionGroup_grant_status_OptionValue_Approved for Payment                | OptionValue   |       900 | unused  | NULL                 |
| 21 | civigrant | OptionGroup_grant_status_OptionValue_Awaiting Information                | OptionValue   |       215 | unused  | NULL                 |
| 18 | civigrant | OptionGroup_grant_status_OptionValue_Eligible                            | OptionValue   |       212 | unused  | NULL                 |
| 19 | civigrant | OptionGroup_grant_status_OptionValue_Ineligible                          | OptionValue   |       213 | unused  | NULL                 |
| 20 | civigrant | OptionGroup_grant_status_OptionValue_Paid                                | OptionValue   |       214 | unused  | NULL                 |
| 17 | civigrant | OptionGroup_grant_status_OptionValue_Submitted                           | OptionValue   |       211 | unused  | NULL                 |
| 22 | civigrant | OptionGroup_grant_status_OptionValue_Withdrawn                           | OptionValue   |       216 | unused  | NULL                 |
| 23 | civigrant | OptionGroup_grant_type                                                   | OptionGroup   |        24 | unused  | NULL                 |
| 15 | civigrant | OptionGroup_mapping_type_OptionValue_Export Grant                        | OptionValue   |       325 | unused  | NULL                 |
| 24 | civigrant | OptionGroup_report_template_OptionValue_CRM_Report_Form_Grant_Detail     | OptionValue   |       261 | unused  | NULL                 |
| 25 | civigrant | OptionGroup_report_template_OptionValue_CRM_Report_Form_Grant_Statistics | OptionValue   |       271 | unused  | NULL                 |
| 34 | civigrant | SavedSearch_CiviGrant_Summary                                            | SavedSearch   |         2 | unused  | NULL                 |
| 35 | civigrant | SavedSearch_CiviGrant_Summary_SearchDisplay_Grant_Tab                    | SearchDisplay |         2 | unused  | NULL                 |
+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
23 rows in set (0.00 sec)

mysql> select * from civicrm_option_value where id in (211,212,213,214,215,216);

+-----+-----------------+----------------------+-------+----------------------+----------+--------+------------+--------+-------------+-------------+-------------+-----------+--------------+-----------+---------------+------+-------+
| id  | option_group_id | label                | value | name                 | grouping | filter | is_default | weight | description | is_optgroup | is_reserved | is_active | component_id | domain_id | visibility_id | icon | color |
+-----+-----------------+----------------------+-------+----------------------+----------+--------+------------+--------+-------------+-------------+-------------+-----------+--------------+-----------+---------------+------+-------+
| 211 |              23 | Submitted            | 1     | Submitted            | NULL     |      0 |          1 |      1 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
| 212 |              23 | Eligible             | 2     | Eligible             | NULL     |      0 |          0 |      2 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
| 213 |              23 | Ineligible           | 3     | Ineligible           | NULL     |      0 |          0 |      3 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
| 214 |              23 | Paid                 | 4     | Paid                 | NULL     |      0 |          0 |      4 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
| 215 |              23 | Awaiting Information | 5     | Awaiting Information | NULL     |      0 |          0 |      5 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
| 216 |              23 | Withdrawn            | 6     | Withdrawn            | NULL     |      0 |          0 |      6 | NULL        |           0 |           0 |         1 |         NULL |      NULL |          NULL | NULL | NULL  |
+-----+-----------------+----------------------+-------+----------------------+----------+--------+------------+--------+-------------+-------------+-------------+-----------+--------------+-----------+---------------+------+-------+
6 rows in set (0.00 sec)

After that step, it still has some metadata about the mgd's (albeit with diff cleanup flags), and the option-values are all gone away.

mysql> select * from civicrm_managed where module='civigrant'; 

+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
| id | module    | name                                                                     | entity_type   | entity_id | cleanup | entity_modified_date |
+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
| 30 | civigrant | Navigation_CiviGrant                                                     | Navigation    |       213 | always  | NULL                 |
| 32 | civigrant | Navigation_CiviGrant_Navigation_Grant_Status                             | Navigation    |       215 | always  | NULL                 |
| 31 | civigrant | Navigation_CiviGrant_Navigation_Grant_Types                              | Navigation    |       214 | always  | NULL                 |
| 26 | civigrant | Navigation_Grants                                                        | Navigation    |       104 | always  | NULL                 |
| 27 | civigrant | Navigation_Grants_Navigation_Dashboard                                   | Navigation    |       105 | always  | NULL                 |
| 29 | civigrant | Navigation_Grants_Navigation_Find_Grants                                 | Navigation    |       107 | always  | NULL                 |
| 28 | civigrant | Navigation_Grants_Navigation_New_Grant                                   | Navigation    |       106 | always  | NULL                 |
| 13 | civigrant | OptionGroup_advanced_search_options_OptionValue_CiviGrant                | OptionValue   |       165 | always  | NULL                 |
| 14 | civigrant | OptionGroup_contact_view_options_OptionValue_CiviGrant                   | OptionValue   |       132 | unused  | NULL                 |
| 16 | civigrant | OptionGroup_grant_status                                                 | OptionGroup   |        23 | always  | NULL                 |
| 33 | civigrant | OptionGroup_grant_status_OptionValue_Approved for Payment                | OptionValue   |       900 | unused  | NULL                 |
| 21 | civigrant | OptionGroup_grant_status_OptionValue_Awaiting Information                | OptionValue   |       215 | unused  | NULL                 |
| 18 | civigrant | OptionGroup_grant_status_OptionValue_Eligible                            | OptionValue   |       212 | unused  | NULL                 |
| 19 | civigrant | OptionGroup_grant_status_OptionValue_Ineligible                          | OptionValue   |       213 | unused  | NULL                 |
| 20 | civigrant | OptionGroup_grant_status_OptionValue_Paid                                | OptionValue   |       214 | unused  | NULL                 |
| 17 | civigrant | OptionGroup_grant_status_OptionValue_Submitted                           | OptionValue   |       211 | unused  | NULL                 |
| 22 | civigrant | OptionGroup_grant_status_OptionValue_Withdrawn                           | OptionValue   |       216 | unused  | NULL                 |
| 23 | civigrant | OptionGroup_grant_type                                                   | OptionGroup   |        24 | always  | NULL                 |
| 15 | civigrant | OptionGroup_mapping_type_OptionValue_Export Grant                        | OptionValue   |       325 | always  | NULL                 |
| 24 | civigrant | OptionGroup_report_template_OptionValue_CRM_Report_Form_Grant_Detail     | OptionValue   |       261 | unused  | NULL                 |
| 25 | civigrant | OptionGroup_report_template_OptionValue_CRM_Report_Form_Grant_Statistics | OptionValue   |       271 | unused  | NULL                 |
| 34 | civigrant | SavedSearch_CiviGrant_Summary                                            | SavedSearch   |         2 | always  | NULL                 |
| 35 | civigrant | SavedSearch_CiviGrant_Summary_SearchDisplay_Grant_Tab                    | SearchDisplay |         2 | always  | NULL                 |
+----+-----------+--------------------------------------------------------------------------+---------------+-----------+---------+----------------------+
23 rows in set (0.00 sec)

mysql> select * from civicrm_option_value where id in (211,212,213,214,215,216);

Empty set (0.00 sec)

Which means that my existing grant records become illegal - because the status_id references non-existent options.

mysql> select id, contact_id, application_received_date, amount_total, status_id from civicrm_grant;
+----+------------+---------------------------+--------------+-----------+
| id | contact_id | application_received_date | amount_total | status_id |
+----+------------+---------------------------+--------------+-----------+
|  1 |        163 | 2022-04-05                |      1000.00 |         5 |
|  2 |          3 | 2022-04-07                |       500.00 |         2 |
+----+------------+---------------------------+--------------+-----------+

lcdservices · 2022-04-12T15:05:22Z

@colemanw I just ran tests going from < 5.47 to 5.48 and previous modifications to the statuses and types were retained (same results as Tim). I didn't go through an incremental test (upgrading to 5.47 and then later to 5.48). But it looks like Tim has the cause of problems there well documented. If you need me to run through that at some point let me know.

Removes managed entities which may have been added during the 5.47 upgrade, to prevent the managed system from automatically deleting them.

colemanw · 2022-04-12T15:34:20Z

Ok so just to clarify, from your POV @lcdservices this fixes your problem (with sites not yet on 5.47).

@totten I've just pushed up another commit to prevent the managed grant statuses from being deleted. Can you re-test the 474_then_48patch upgrade?

lcdservices · 2022-04-12T15:41:54Z

@colemanw yes, this fixes the problem I experienced.

colemanw · 2022-04-12T15:43:55Z

Note: once this PR is ✔️ it will also need to be done for 5.49 -> master.

totten · 2022-04-12T22:01:35Z

My local was happy with the upgrades but CI wasn't -- I think it's because we needed the set-version.php updates. Added that on.

I retested, and it looks better. 👍

Upgrade Path	Grant Types	Grant Statuses
5.45 => `48patch`	good ✔️	good ✔️
5.45 => `474_then_48patch`	good ✔️	As good as can be. The 5.47.4 interlude reset the status-names, and we can't fix that here. If they did any cleanup while running 5.47.x, then that cleanup will still carry-forward.
5.47 => `48patch`	good ✔️	good ✔️

totten · 2022-04-12T22:11:27Z

Merge on pass.

Added forward-port for 5.49-rc: #23179

civibot bot added the 5.48 label Apr 8, 2022

CiviGrant - Remove unused managed entities during upgrade

ec44d7e

Removes managed entities which may have been added during the 5.47 upgrade, to prevent the managed system from automatically deleting them.

Set version to 5.48.1

fd486b2

colemanw added the merge ready PR will be merged after a few days if there are no objections label Apr 12, 2022

totten mentioned this pull request Apr 12, 2022

(dev/core#3161) CiviGrant - Cleanup managed entities (5.49-rc) #23179

Merged

totten added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Apr 12, 2022

eileenmcnaughton merged commit 1aedd1e into civicrm:5.48 Apr 12, 2022

eileenmcnaughton deleted the managedGrantEntities branch April 12, 2022 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CiviGrant - Cleanup managed entities - fixes dev/core#3161 #23140

CiviGrant - Cleanup managed entities - fixes dev/core#3161 #23140

colemanw commented Apr 8, 2022 •

edited

Loading

civibot bot commented Apr 8, 2022

totten commented Apr 12, 2022 •

edited

Loading

totten commented Apr 12, 2022

lcdservices commented Apr 12, 2022

colemanw commented Apr 12, 2022

lcdservices commented Apr 12, 2022

colemanw commented Apr 12, 2022

totten commented Apr 12, 2022

totten commented Apr 12, 2022

CiviGrant - Cleanup managed entities - fixes dev/core#3161 #23140

CiviGrant - Cleanup managed entities - fixes dev/core#3161 #23140

Conversation

colemanw commented Apr 8, 2022 • edited Loading

Overview

Technical Details

civibot bot commented Apr 8, 2022

totten commented Apr 12, 2022 • edited Loading

totten commented Apr 12, 2022

lcdservices commented Apr 12, 2022

colemanw commented Apr 12, 2022

lcdservices commented Apr 12, 2022

colemanw commented Apr 12, 2022

totten commented Apr 12, 2022

totten commented Apr 12, 2022

colemanw commented Apr 8, 2022 •

edited

Loading

totten commented Apr 12, 2022 •

edited

Loading