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.

Sign up for GitHub

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

Jump to bottom

dev/1640 New feature - adds a checkbox to the update contribution status form to determine if a receipt is sent #16742

Merged
merged 1 commit into from
Mar 21, 2020
Merged

dev/1640 New feature - adds a checkbox to the update contribution status form to determine if a receipt is sent #16742

merged 1 commit into from
Mar 21, 2020

Conversation

jaapjansma
Copy link
Contributor

Overview

Add an option to the update contribution status to send an e-mail receipt or not.

See: https://lab.civicrm.org/dev/core/issues/1640

Before

Screenshot from 2020-03-10 22-39-20

After

Screenshot from 2020-03-10 22-58-49

Comments

The code in this bit of PR contains a comment to move to the order api.
I thought about change that but I was unsure because it might affect more than this simple change.

@civibot
Copy link

civibot bot commented Mar 10, 2020

(Standard links)

@civibot civibot bot added the master label Mar 10, 2020
@kartik1000
Copy link
Contributor

@eileenmcnaughton Don't you think we should add a test here to ensure the working of checkbox and see accordingly if mail is sent or not? Otherwise the change looks good👍

@eileenmcnaughton eileenmcnaughton changed the title dev/1640 dev/1640 New feature - adds a checkbox to the update contribution status form to determine if a receipt is sent Mar 12, 2020
@eileenmcnaughton
Copy link
Contributor

@kartik1000 yes - we really do need a test on this - I thought about whether we could squeeze it through without but really

  1. it's a new feature and
  2. the code being called is likely to be altered at some point as calling the IPN class from here really is just a very old hack.

On the bright side there is a test already on the form that could be a starting point CRM_Contribute_Form_Task_StatusTest (if you are feeling keen you could tackle it)

I just updated the PR title as it wasn't descriptive.

I'm a little unsure about whether this requires documentation as I'm not sure what the current default is - ie if it's currently sending an email & the default here is to still send an email then most users wouldn't notice

@jaapjansma
Copy link
Contributor Author

@eileenmcnaughton the default behavior was that it sends out e-mails. The new default is that the checkbox is ticked and therefor would also send out e-mails.

@jaapjansma
Copy link
Contributor Author

test this please

@jaapjansma
Copy link
Contributor Author

I can tell that this patch has been tested by one of our clients and is now applied in production.

@jaapjansma
Copy link
Contributor Author

test this please

@jaapjansma jaapjansma force-pushed the dev-1640 branch 2 times, most recently from 363b74e to a7349f8 Compare March 21, 2020 16:17
demeritcowboy
demeritcowboy reviewed Mar 21, 2020
View reviewed changes
tests/phpunit/CRM/Contribute/Form/Task/StatusTest.php Outdated
$this->_individualId = $this->individualCreate();
$form = new CRM_Contribute_Form_Task_Status();

$mut = new CiviMailUtils($this, TRUE);
$mut->clearMessages();
$mut->assertMailLogEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 36 and 85 don't hurt but don't really do anything. It would just be testing the mail utils itself not the feature being added. Sorry that might have come from the first version of the chat post where I made a typo.

@jaapjansma
Copy link
Contributor Author

@demeritcowboy are you able to review this PR and the test? Thanks

@demeritcowboy
Copy link
Contributor

Yep I can take a look.

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
      • As noted above the default is the same as before.
    • [r-run] PASS
      • It's not because of the PR just noticing it's very chatty in ConfigAndLog. Not warnings, more like as if verbose logging was on.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] PASS
      • Can remove the needs-test label.
    • [r-test] PASS

@colemanw colemanw merged commit 381b622 into civicrm:master Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@demeritcowboy demeritcowboy demeritcowboy left review comments

Assignees
No one assigned
Labels
has-test master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jaapjansma @kartik1000 @eileenmcnaughton @demeritcowboy @colemanw