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: improve return value of `Give_Donate_Form::get_goal()` #4020

Closed
andrewminion-luminfire opened this issue Feb 25, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@andrewminion-luminfire
Copy link

commented Feb 25, 2019

Bug Report

User Story

As a developer, I want Give_Donate_Form’s get_goal() method to return false if the goal is disabled for a form so that I can use it to determine whether a goal is set without using get_post_meta( $form_id, '_give_goal_option', true ).

Current Behavior

Currently, get_goal() returns the meta value for _give_set_goal regardless of whether the goal is enabled or not. By default, the value is 1.000000. A default goal of $1.00 is unlikely but possible.

Expected Behavior

In my opinion, get_goal() should first check to see if the goal is enabled and if not, return false.

Bug Type

  • This bug describes functionality that once worked as expected in version X.X.X.
  • This bug describes functionality that never worked as expected.
  • I am not sure whether this functionality ever worked as expected.

Steps to Reproduce

  1. Write this code:
$form = new Give_Donate_Form( $form_id ); 
$goal = $form->get_goal();
if ( isset( $goal ) ) {
  // Do stuff.
}

Possible Solution

Add a a check to see if the goal is set or not.

Acceptance Criteria

  • If a goal is enabled, get_goal() should return the goal.
  • If a goal is not enabled, get_goal() should return false, not the default value of 1.000000
@kevinwhoffman

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Hey @andrewminion-luminfire thanks for the request. We're going to take a look at that return value and improve it in Give 2.4.2.

@kevinwhoffman kevinwhoffman changed the title Give_Donate_Form()->get_goal() should return `false` if the goal is not enabled. fix: improve return value of `Give_Donate_Form()->get_goal()` Feb 26, 2019

@kevinwhoffman kevinwhoffman changed the title fix: improve return value of `Give_Donate_Form()->get_goal()` fix: improve return value of `Give_Donate_Form::get_goal()` Feb 26, 2019

@ravinderk

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

Slack Chat Summary

Participants: @DevinWalker @kevinwhoffman @ravinderk
Topic: Should we return multiple types of result from get_goal
Result: Best practice to return one type if possible. We don’t use return type declarations, but they were introduced in PHP 7.0 and allow you to declare the return type in the function signature. https://wiki.php.net/rfc/return_types
Prevents errors with unexpected return values and makes testing easier.

This is still open for discussion whether we can create has_goal function instead of handling user case in get_goal function.

@andrewminion-luminfire

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

Adding has_goal would work for me.

Returning only strings from get_goal makes sense, but in my opinion, it should return an empty string if the goal is disabled, rather than the goal value. @ravinderk @DevinWalker @kevinwhoffman

DevinWalker added a commit that referenced this issue Feb 27, 2019

Merge pull request #4022 from impress-org/issue/4020
fix: improve return value of `Give_Donate_Form::get_goal()` #4020

ravinderk added a commit that referenced this issue Mar 16, 2019

fix: display the correct percentage-based progress
This bug introduced by fix of this issue: #4020

ref #4049

ravinderk added a commit that referenced this issue Mar 25, 2019

fix: display the correct percentage-based progress
This bug introduced by fix of this issue: #4020

ref #4049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.