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

CRM-21199, removed JS to hide invoice page setting #11003

Merged
merged 1 commit into from
Sep 23, 2017

Conversation

pradpnayak
Copy link
Contributor

@pradpnayak pradpnayak commented Sep 19, 2017


Overview

Currently 'Default invoice payment page' setting under CiviContribute Settings shows up when 'Enable Deferred Revenue' is enabled. These two settings are independent of each other. We should remove the dependency on Enable Deferred Revenue since there is no logical link. People not using deferred revenue could benefit from this functionality.

Before

screen shot 2017-09-20 at 4 28 17 am

After

screen shot 2017-09-20 at 4 28 49 am

----------------------------------------
* CRM-21199: Remove dependancy for  'Default invoice payment page'
  https://issues.civicrm.org/jira/browse/CRM-21199
@monishdeb
Copy link
Member

Test build failure is unrelated

Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

I have reviewed this PR locally on the following criteria and I think it's good to merge.

  • ✅ It's a worthy pursuit.
  • ✅ It works as advertised.
  • ✅ It makes sense for all users who may be affected.
  • ✅ It's unlikely to confuse users.
  • ✅ It doesn't appear to cause bugs or unwanted side effects.
  • ✅ It doesn't appear to introduce security vulnerabilities.
  • ✅ New code is either: (a) not present, or (b) readable and sensible.
  • ✅ Acceptance criteria are either: (a) not stated, or (b) met.
  • ✅ Significant new functionality is either: (a) not present, or (b) covered by new unit tests.
  • ✅ New documentation is either: (a) not needed, or (b) provided within this PR, or (c) provided in a separate PR which already exists.

Ping @eileenmcnaughton @monishdeb

@seamuslee001
Copy link
Contributor

Jenkins test this please

@eileenmcnaughton
Copy link
Contributor

I'm all for removing code & complexity. I'm happy the review @seanmadsen has done is appropriate to the issue, which only affects one admin screen

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.

6 participants