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

core#1836 - restrict scheduled reminder 'Also Include' to non-events #23101

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

MegaphoneJon
Copy link
Contributor

Overview

This is a rewrite of #20425. As @magnolia61 pointed out - it was a good idea, and I'm not sure why I closed the PR. It was probably accidental, because the issue is still a problem.

Before

In Scheduled Reminders, you can select "Also Include" recipients with event reminders, even though that's broken.

After

You can't select "Also Include" on event reminders. If you selected "Also Include" that choice is removed.

Comments

@magnolia61 you seemed to have an interest in this?

For testing purposes - it's best to bring up a site that has the patch and one that doesn't in adjoining tabs, and observe the difference in behavior when the same options are selected.

@civibot
Copy link

civibot bot commented Apr 5, 2022

(Standard links)

@civibot civibot bot added the master label Apr 5, 2022
@eileenmcnaughton
Copy link
Contributor

Test error has affected many, but not all, test runs today - we can test this again in the hope of getting rid of the red x distraction

Stacktrace
CRM_Contribute_Form_ContributionTest::testSubmitCreditCardPayPal
GuzzleHttp\Exception\ServerException: Server error: POST [https://api-3t.sandbox.paypal.com/nvp](https://api-3t.sandbox.paypal.com/nvp%60) resulted in a 500 Internal Server Error response:

@eileenmcnaughton
Copy link
Contributor

test this please

1 similar comment
@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR and in gitlab.
    • 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: We created a scheduled reminder in dmaster and in the test environment of this PR. In dmaster we could select Also include for event reminders. But in this PR we could not.
  • 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 is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

Note we do think that this PR could be merged. However it would be better to tackle the underlying problem and solve the issue of sending scheduled reminders for events which also have also include set.

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

@jaapjansma jaapjansma added the merge ready PR will be merged after a few days if there are no objections label Apr 11, 2022
@eileenmcnaughton
Copy link
Contributor

Thanks @jaapjansma - since this doesn't touch the php layer I agree no test is OK

@eileenmcnaughton eileenmcnaughton merged commit 5c0e929 into civicrm:master Apr 11, 2022
totten added a commit to totten/civicrm-core that referenced this pull request May 30, 2022
… Include"

After reading civicrm#23445 and civicrm#23101, I wanted to see the test-coverage for "Also
Include".  There's a little...  but not a lot.  There was an interesting-but-inert
test-case.

Before
------

* Test-case is commented-out
* Comments indicate a vague sense of confusion

After
-----

* Test-case is enabled
* Comments indicate a well-developed, robust sense of confusion
totten added a commit to totten/civicrm-core that referenced this pull request May 30, 2022
… Include"

After reading civicrm#23445 and civicrm#23101, I wanted to see the test-coverage for "Also
Include".  There's a little...  but not a lot.  There was an interesting-but-inert
test-case.

Before
------

* Test-case is commented-out
* Comments indicate a vague sense of confusion

After
-----

* Test-case is enabled
* Comments indicate a well-developed, robust sense of confusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
3 participants