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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve logic for donor and user profile disconnection #4113

Closed
denisjacobi opened this issue May 21, 2019 · 0 comments

Comments

Projects
None yet
3 participants
@denisjacobi
Copy link

commented May 21, 2019

Bug Report

User Story

Sigh, user stories... 馃檮... Ok. As a developer, I want code without Bugs 馃槒

Current Behavior

This does not impact functionality.

However there is a logic bug in includes/admin/donors/donor-actions.php in function give_disconnect_donor_user_id() line 313 of Give 2.4.7 in:

if ( ! $donor->update( $donor_args ) ) {
		update_user_meta( $user_id, '_give_is_donor_disconnected', true );
		update_user_meta( $user_id, '_give_disconnected_donor_id', $donor->id );
		$donor->update_meta( '_give_disconnected_user_id', $user_id );

		$output['success'] = true;

	} else {
		$output['success'] = false;
		give_set_error( 'give-disconnect-user-fail', __( 'Failed to disconnect user from donor.', 'give' ) );
	}

When disconnecting a donor in the admin panel if ( ! $donor->update( $donor_args ) ) does the actual update_meta to set the user_id meta_value to 0. The update will go through successfully and it will always return true. Since you check for false, it will never go into that block and instead incorrectly do the else block which is intended for failure. Wy the error that鈥檚 set doesn鈥檛 show, I don鈥檛 know. Didn鈥檛 look into that.

How I realized this is that I was wondering why in includes/admin/donors/donor-functions.php in function give_connect_user_donor_profile() you are checking and deleting metas:

_give_disconnected_user_id
_give_is_donor_disconnected
_give_disconnected_donor_id

Looking at the database these meta keys are never present anywhere for a user. That is because of the incorrect true/false check in give_disconnect_donor_user_id().

Honestly, I can鈥檛 even figure out why you wanted to set those 3 metas in the first place. I don鈥檛 see any other functions that would use them, so they seem redundant??

Bug Type

  • I am not sure whether this functionality ever worked as expected.

Steps to Reproduce

  1. Look at the code.

Possible Solution

Though I didn't test it, I'd assume change the if statement to

if ( $donor->update( $donor_args ) ) {

or maybe rethink what those _give_disconnected_user_id, etc were supposed to be used for?

Acceptance Criteria

  • Improve logic for user and donor profile disconnection
  • Fix the new bug found while investigating the issue - Reconnect user & donor disconnection email is not sent!
  • Testing

Environment

Give version 2.4.7

@mehul0810 mehul0810 self-assigned this May 21, 2019

@ravinderk ravinderk added this to the May 2019 milestone May 21, 2019

@mehul0810 mehul0810 changed the title Bug in give_disconnect_donor_user_id() fix: improve logic for donor and user profile disconnection May 22, 2019

ravinderk added a commit that referenced this issue May 22, 2019

Merge pull request #4115 from impress-org/issue/4113
fix: improve logic for donor and user profile disconnection #4113
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.