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 error when adding activities from Search Builder #15522

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

MegaphoneJon
Copy link
Contributor

Overview

When you use the "Add Activity" task from Search Builder, you get an error on submitting the activity. Exact replication steps are on the issue: https://lab.civicrm.org/dev/core/issues/1323

Before

Yellow Screen of Death.

After

No error.

Technical Details

When you submit the activity, CiviCRM tries passing you back to the search screen you just came from. However, it doesn't handle Search Builder.

Comments

I'm tagging this as easy-review because of the simplicity of the steps to replicate, the limited scope of the class being altered, and the obvious correctness of the fix :)

@civibot
Copy link

civibot bot commented Oct 16, 2019

(Standard links)

@alifrumin
Copy link
Contributor

@eileenmcnaughton this looks ready to be merged to me!

  • General standards

    • Explain (r-explain)
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: followed instructions in the overview on the jenkins build. Works as expected.
  • 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.
    • Maintainability (r-maint)
      • PASS: The change sufficiently improves test coverage, or the change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@eileenmcnaughton
Copy link
Contributor

Thanks @MegaphoneJon & @alifrumin - looks good to me

@eileenmcnaughton eileenmcnaughton merged commit f2a37c0 into civicrm:master Oct 16, 2019
@MegaphoneJon MegaphoneJon deleted the core-1323 branch November 3, 2019 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants