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

dev/core#163 Improve inclusion of disabled groups when getting all ma… #12277

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

seamuslee001
Copy link
Contributor

…ilings user is able to access

@eileenmcnaughton this is a minor improvement in the code, i had to back out of the previous version from AUG's prod because i think it was still interpreting the '' in strange way but this version has been successfully tested on a dev site.

@civibot
Copy link

civibot bot commented Jun 7, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 hmm - we should probably check why the '' doesn't work

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton

I was noticing queries getting generated like ```
$Query = SELECT a.id as id, a.title as `title`
FROM civicrm_group a
WHERE ((`a`.`id` IS NULL OR (`a`.`id` IN (4106,4114,4643,4993,391,427,683,876,877,878,879,882,884,885,886,887,888,889,890,891,892,893,895,896,897,898,899,900,902,903,904,905,906,907,908,909,910,914,916,917,918,919,920,921,923,924,928,929,931,932,933,934,935,936,937,939,941,942,943,944,946,947,949,952,953,954,958,963,966,1035,1037,1087,1097,1130,1134,1135,1153,1180,1182,1183,1359,1361,1364,1384,1401,1411,1431,1432,1433,1457,1465,1500,1544,1589,1607,1662,557,559,564,1674,1675,1715,1730,1734,1735,1736,1737,1738,1739,1752,1766,1768,1782,1809,1816,1829,1841,2081,2162,2204,2322,2330,2336,2347,2349,2369,2370,2376,2386,2392,2404,2405,2444,2445,2446,2447,2448,2458,2513,2514,2519,2520,2627,2671,2748,2770,2772,2784,2789,2790,2793,2794,2795,2796,2797,2798,2799,2800,2801,2809,2817,2819,2820,2823,2835,2839,2859,2862,2873,2874,2877,2892,2896,2898,2901,2902,2904,2938,2967,2968,3064,3065,3066,3067,3068,3069,3087,3094,3123,3136,3137,3138,3147,3148,3149,3150,3203,3207,3241,3264,1554,3277,3288,3295,3301,3303,3304,3305,3306,3307,3308,3309,3312,3313,3314,3315,3316,3317,3318,3320,3321,3322,3323,3324,3325,3326,3327,3328,3329,3330,3331,3332,3333,3334,3335,3337,3338,3339,3340,3341,3342,3343,3344,3347,3351,3352,3370,3371,3372,3373,3374,3375,3376,3377,3378,3379,3380,3381,3382,3383,3384,3385,3387,3388,3389,3390,3392,3393,3414,3415,3416,3418,3419,3420,3421,3422,3423,3424,3425,3426,3427,3428,3429,3430,3431,3432,3433,3434,3436,3437,3440,3441,3442,3443,3446,3447,3448,3478,3479,3480,3481,3482,3483,3484,3485,3486,3487,3488,3489,3490,3491,3492,3493,3494,3495,3496,3497,3498,3499,3500,3501,3502,3504,3512,3513,3516,3526,3527,3530,3539,3548,3549,3550,3561,3562,3563,3569,3570,3571,3572,3574,3577,3578,3580,3602,3606,3607,3611,3630,3632,3634,3635,3636,3637,3638,3639,3640,3641,3642,3643,3644,3645,3646,3647,3648,3649,3650,3651,3652,3653,3654,3655,3658,3659,3678,3679,3680,3681,3682,3683,3684,3685,3686,3687,3688,3689,3690,3691,3692,3693,3694,3695,3696,3697,3698,3699,3700,3702,3703,3704,3705,3707,3708,3713,3717,3725,3726,3734,3743,3745,3746,3747,3756,3757,3758,3762,3763,3764,3766,3767,3770,3771,3772,3773,3774,3783,3786,3787,3788,3789,3790,3791,3792,3793,3794,3795,3796,3797,3798,3799,3800,3801,3802,3803,3804,3817,3818,3833,3847,3848,3849,3850,3851,3853,3854,3855,3858,3857,3859,3860,3861,3862,3863,3864,3873,3875,3876,3877,3899,3900,3932,3978,3989,3991,3994,3995,3996,3997,3998,3999,4002,4006,4075,4076,4078,4079,4080,4081,4082,4083,4084,4085,4086,4087,4088,4089,4095,4096,4097,4098,4099,4115,4116,4117,4118,4119,4120,4121,4122,4123,4124,4126,4127,4128,4129,4130,4131,4132,4133,4134,4136,4137,4138,4139,4140,4141,4142,4143,4144,4150,4199,4200,4201,4135,4125,4226,4238,4239,4243,4313,4321,4369,4370,4396,4397,4398,4415,4444,4445,4446,4449,4458,4460,4461,4464,4470,4485,4487,4495,4516,4518,4519,4520,4528,4529,4530,4538,4584,4590,4606,4627,4628,4630,4633,4635,4636,4639,4640,4649,4662,4666,4667,4677,4678,4679,4680,4685,4702,4738,4739,4741,4742,4748,4751,4753,4757,4306,4305,4303,4777,4275,4773,4778,4277,4763,4771,4278,4263,4765,4283,4762,4284,4769,4780,4781,4770,4782,4783,4787,4784,4766,4767,4768,4279,4755,4281,4786,4775,4274,4280,4273,4813,4818,4830,4841,4853,4856,4859,4861,4874,4876,4886,4905,4909,4914,4917,4920,4921,4922,4923,4924,4942,4991,4992,4995,5010,5017,5021,5022,5023,5028,5037,5066,5068,5072,5079,5080,5081,5087,5088,5089,5092,5114,5115,5132,5141,5144,5147,5149,5150,5151,5152,5153,5155,5156,5158,5159,5161,5162,5163,5171,5165,5166,5167,5168,5169,5174,5173,5176,5177,5178,5179,4190,1248,4211,5212,5263,5313,5314,5316,5317,5319,5322,5326,5327,5328,5329,5332,5409,5423,5474,5475,5487,5531,5539,5547,5548,5591,5592,5612,743,5624,5632,5633,5634,5635,5639,5651,5656,5675,5694,5695,5713,5723,5738,5742,5747,5748,5752,5753,5755,5757,5758,5760,5761,5768,5773,5774,5775,5789,5790,5791,5792,5805,5806,5812,5821,5822,5834,5844,5860,5861,5862,5863,5864,5869,5874,5875,5890,5893,5898,5902,5903,5909,5910,5911,5915,5943,5945,5981,5982,5983,5991,5993,6000,6009,6010,6011,6017,6058,6059,6060,6072,6073,6099,6174,6177,6178,6179,6180,6210,6215,6248,6268,6273,6274,6299,6320,6343,6354,6357)))) AND (a.is_active = "")

@seamuslee001
Copy link
Contributor Author

running SELECT * FROM greens_contact.civicrm_group WHERE is_active = '' only returns disabled groups locally whereas the intent was to get both active and disabled groups

@eileenmcnaughton
Copy link
Contributor

hmm - I feel like that's wrong behaviour though & we should bang in some tests - we expect '' to be ignored by the api don't we?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton its a tricky one to me because in some cases '' feels like it should be allowable e.g. your searching for say a blank last_name in the db or something that is not null but just an empty string

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 I think the above query is coming from multisite extension - however I now think just not passing is_active is correct here

@seamuslee001
Copy link
Contributor Author

ok will update PR

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton PR updated now

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@seamuslee001
Copy link
Contributor Author

(@eileenmcnaughton triggering a retest following merging of your group pr just in case)

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 the PR this is a follow up on is not in 5.3 rc is it?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton it looks like it is in 5.3 should i put this against the RC?

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 yep

… mailing accessable by a user by removing is_active filter alltogether
@seamuslee001 seamuslee001 changed the base branch from master to 5.3 June 11, 2018 00:23
@seamuslee001
Copy link
Contributor Author

done @eileenmcnaughton

@seamuslee001
Copy link
Contributor Author

Merging as per the tag @eileenmcnaughton btw i confirmed that the last master version (with your pr merged) also passed

@seamuslee001 seamuslee001 merged commit c74f20c into civicrm:5.3 Jun 11, 2018
@seamuslee001 seamuslee001 deleted the dev_core_163 branch June 11, 2018 03:40
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.

2 participants