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 for #140, changing the logic to remove or add roles only if neede… #623

Open
wants to merge 4 commits into
base: 7.x-master
Choose a base branch
from

Conversation

sadashivdalvi
Copy link
Contributor

…d and always use google api instead of delete

…f needed and always use google api instead of delete
@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot civibot bot added the 7.x-master label Sep 9, 2020
@civibot
Copy link

civibot bot commented Sep 9, 2020

(Standard links)

@petednz
Copy link

petednz commented Feb 3, 2021

@jitendrapurohit can you look over this?

@aydun
Copy link
Contributor

aydun commented Feb 3, 2021

@sadashivdalvi What is the problem you are addressing? You mention '#140' but in which system? The PR that is automatically linked does not look relevant. I don't understand the comment 'always use google api instead of delete' ... the code makes no use of google api's.

@sadashivdalvi
Copy link
Contributor Author

I have a system which performs certain operation whenever a role is added or removed, this operation is to sync some external system.

What currently happens is, system removes all roles (using db delete which doesn't invoke any hooks, etc) and then adds roles which actually invokes the external system unnecessarily, Instead of that civicrm code should identify whether we need a change in role and use api to save roles.

I am using this patch on production since I submitted the PR and it's working fine.

"google" was a typing mistake, I meant use api to save role changes.

Thanks,
Sadashiv

@aydun
Copy link
Contributor

aydun commented Feb 3, 2021

Ok, that makes sense. The changes are smaller than the diff suggests at first sight. I have not tested it, but the code changes look good to me.
Let's see what Jitendra says.


if (empty($contactMemberships) && !empty($rid)) {
db_delete('users_roles')->condition('uid', $uid)->condition('rid', $rid, $roleCondition)->execute();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this no longer happening now? I just replicated the following problem which wasn't happening with the previous code.

  • Create 2 member sync rule. Eg role1 => memtype1 & role2 => memtype2.
  • Create a contact (with drupal user) and add a membership of type = memtype1.
  • Sync the rule. role1 is added to the user (Correct).
  • Delete the membership memtype1 from the contact and add the membership of type = memtype2.
  • Sync the rule again. role2 is added (Correct). But role1 is not removed from the contact (Incorrect).

As memtype1 is not associated with the civi contact, role1 should have been removed after the sync. It was working as per the above condition.

@sadashivdalvi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jitendrapurohit can you please recheck, I missed that case, which I fixed by adding additional check now.

foreach ($memberroles as $membership_type_id => $rolerule) {
  if (user_has_role($rolerule['rid'], $account) && !in_array($rolerule['rid'], $addRoles)) {
    // This will only happen if the membership is deleted.
    $expRoles[] = $rolerule['rid'];
  }
}

Simple check that if the user has a role in the rule and doesn't have it addRoles then remove that role.

Have also improved the saving logic by moving the save out of loops, something I missed last time.

Thanks,
Sadashiv

Copy link
Contributor Author

@sadashivdalvi sadashivdalvi left a comment

Choose a reason for hiding this comment

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

I did find on my site that we do need a fix from https://issues.civicrm.org/jira/browse/CRM-16000 i.e. #316 Should I include that patch here so that the code becomes complete and functional.

I have this deployed on my production site.

Thanks,
Sadashiv


if (empty($contactMemberships) && !empty($rid)) {
db_delete('users_roles')->condition('uid', $uid)->condition('rid', $rid, $roleCondition)->execute();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jitendrapurohit can you please recheck, I missed that case, which I fixed by adding additional check now.

foreach ($memberroles as $membership_type_id => $rolerule) {
  if (user_has_role($rolerule['rid'], $account) && !in_array($rolerule['rid'], $addRoles)) {
    // This will only happen if the membership is deleted.
    $expRoles[] = $rolerule['rid'];
  }
}

Simple check that if the user has a role in the rule and doesn't have it addRoles then remove that role.

Have also improved the saving logic by moving the save out of loops, something I missed last time.

Thanks,
Sadashiv

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.

5 participants