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#1386 Allow advanced search for contributions without a soft credit related #15834

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

francescbassas
Copy link
Contributor

@civibot
Copy link

civibot bot commented Nov 12, 2019

(Standard links)

@civibot civibot bot added the master label Nov 12, 2019
@francescbassas
Copy link
Contributor Author

test this please

1 similar comment
@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

Betty and I are reviewing PR's and we came across this PR.

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR and in the issue.
    • User impact (r-user)
      • ISSUE: The change would noticeably impact the user-experience but does not require training.
      • COMMENTS: We would suggest renaming the list of options to:
        • Contributions Only
        • Soft Credits Only
        • Soft Credits with related Hard Credit Contributions and their related soft credit
        • Contributions (only) without a soft credit
        • Both All
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We have tested this PR and it works.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
      • COMMENTS: We have given this pass. However the added option has a name which is unclear to us.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@francescbassas could have a look at our remark on the user impact? Are you able to rename the options so it becomes a bit more clear.

francescbassas added a commit to francescbassas/civicrm-core that referenced this pull request Feb 19, 2020
francescbassas added a commit to francescbassas/civicrm-core that referenced this pull request Feb 19, 2020
@francescbassas
Copy link
Contributor Author

Options renamed. Thanks for the review.

@jaapjansma
Copy link
Contributor

Betty and I have tested the changes and this pr is ready to be merged.
However we do think it might be good to refactor/redesign the find contributions screen and how the drop down of soft credits fits into it However that is a lot of work and out of the scope of this PR.

@eileenmcnaughton or @mattwire could one of you merge this PR?

@seamuslee001 seamuslee001 merged commit ee4ac4e into civicrm:master Feb 21, 2020
@eileenmcnaughton
Copy link
Contributor

I actually disagree that this change is trivial enough it doesn't require tests- we have had no end of problems with soft- credit related search features & I think in general adding any new feature to search or reports requires a test.

However, it's merged now - and we got the queue down a bit

seamuslee001 added a commit to seamuslee001/civicrm-core that referenced this pull request Feb 24, 2020
@eileenmcnaughton
Copy link
Contributor

Update - @seamuslee001 has written the missing test!

eileenmcnaughton added a commit that referenced this pull request Feb 24, 2020
magnolia61 pushed a commit to magnolia61/civicrm-core that referenced this pull request Mar 3, 2020
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.

5 participants