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#1291 Use name instead of label to check contribution status #15376

Merged
merged 1 commit into from
Oct 5, 2019

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Oct 2, 2019

Overview

This will fail if the label is changed (eg. different language). Now it won't.

https://lab.civicrm.org/dev/core/issues/1291

Before

Check label

After

Check name

Technical Details

Need to check name not label.

Comments

@mlutfy This is similar to other translation fixes you've done - can you check?

@civibot
Copy link

civibot bot commented Oct 2, 2019

(Standard links)

@civibot civibot bot added the master label Oct 2, 2019
@mlutfy
Copy link
Member

mlutfy commented Oct 2, 2019

Seems OK on code review. How can I reproduce?

@mattwire
Copy link
Contributor Author

mattwire commented Oct 3, 2019

@mlutfy I'm not sure how you could reproduce - I just came across it when tracing another issue and thought "that will cause problems".

@jitendrapurohit
Copy link
Contributor

Agree with the change made here. It looks like this file was recently changed and refactored here - https://github.com/civicrm/civicrm-core/pull/15124/files. This PR seems to revert that line to correctly retrieve the status value by name. +1 to get this merged

@mlutfy
Copy link
Member

mlutfy commented Oct 5, 2019

Merging based on Jitendra's review, and also on #15391 from @samuelsov.

@mlutfy mlutfy merged commit d37341f into civicrm:master Oct 5, 2019
@mlutfy mlutfy changed the title Use name instead of label to check contribution status dev/core#1291 Use name instead of label to check contribution status Oct 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants