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

Cleanup tracking on group.load #20310

Merged
merged 1 commit into from
May 17, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 15, 2021

Overview

Cleanup tracking on group.load

Before

There is a static to determine if it has already been called in the php process and a $force param to by pass it.
Note that if the load is successful the group is updated with detail about it. If not it isn't. The static returns just because we tried whereas it makes more sense to skip based on whether or not the group needs a reload at that moment.

After

Static gone, force deprecated.

Technical Details

  1. deprecate force parameter
  2. ditch the static & let the db lookups speak for themselves

The force paramter seems to mostly be about the static & probably only exists to support tests.
The static doesn't make sense now because either they group needs loading or it doesn't.
The fact the process might have tried and succeeded or failed before doesn't need
to be recorded in the static

Comments

I suspect some unit tests might need to be adjusted beyond just dropping force but will see the outcome

@civibot
Copy link

civibot bot commented May 15, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 @colemanw I think if we can merge this it will make other tests less flakey

@mattwire
Copy link
Contributor

I've loaded this onto a client site for testing

1) deprecate force parameter
2) ditch the static & let the db lookups speak for themselves

The force paramter seems to mostly be about the static & probably only exists to support tests.
The static doesn't make sense now because either they group needs loading or it doesn't.
The fact the process might have tried and succeeded or failed before doesn't need
to be recorded in the static

Cleanup up progress tracking in Group::load

Fix test to do things in a logical order so contacts exist when the group is created
@seamuslee001
Copy link
Contributor

This makes sense to me the actual code changes look pretty minor here and the test coverage is good here and the test changes are just re-ordering when certain test objects are created. MOP

@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire - I am expecting this merge will permit this to pass #20243 which you might like to add to your client site too - it should have a least a potential performance improvement in some edge cases

@eileenmcnaughton eileenmcnaughton merged commit bde0545 into civicrm:master May 17, 2021
@eileenmcnaughton eileenmcnaughton deleted the group_lock branch May 17, 2021 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants