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

Fix cache clearing when import table is changed #25774

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix cache clearing when import table is changed

Before

  1. enable Civi-Import
  2. Start an import - upload the csv then go back & re-do the csv upload
  3. navigate to the import search - as long as caches have not otherwise been cleared the api call failure will be visible in the console

After

The cache is cleared whenever the table is changed, test coverage

Technical Details

We were clearing the cache on create & delete, but not when the table was updated - which meant we could hit errors when the cache was trying to access a now-deleted table

Comments

@civibot
Copy link

civibot bot commented Mar 9, 2023

(Standard links)

@@ -146,7 +146,7 @@ public static function getStatuses(): array {
* @return array
*/
public static function getTypes(): array {
$types = Civi::cache()->get('UserJobTypes');
$types = Civi::cache('metadata')->get('UserJobTypes');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only thought this was relevant - but we should use the metadata cache here for consistency

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Mar 9, 2023
@eileenmcnaughton
Copy link
Contributor Author

@totten should this be merge on pass or do we need to wait a few more days after having group-discussed

@totten
Copy link
Member

totten commented Mar 10, 2023

@eileenmcnaughton Don't have to wait for a few days. I flagged merge ready because there was chance that Coleman was affected/could feedback. But yeah, convo indicated it's OK to merge.

Aside: for the test fail, here's a targetted rerun: https://test.civicrm.org/job/CiviCRM-Manual-Test/47/

@eileenmcnaughton eileenmcnaughton force-pushed the import_cache branch 2 times, most recently from 216b80d to 580aee7 Compare March 10, 2023 00:55
@eileenmcnaughton
Copy link
Contributor Author

@totten ah no - I don't think @colemanw was knowingly affected - I was trying to warn him in case lack-of-this-being-merged cost him a bunch of time if he did hit it

@eileenmcnaughton
Copy link
Contributor Author

It went stale during that test run - rebased

@eileenmcnaughton
Copy link
Contributor Author

This is passing so merging as discussion suggests it really is merge-on-pass

@eileenmcnaughton eileenmcnaughton merged commit 0b35e5f into civicrm:master Mar 10, 2023
@eileenmcnaughton eileenmcnaughton deleted the import_cache branch March 10, 2023 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants