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

[Ref] extract function to getEmailDefaults #21067

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[Ref] extract function to getEmailDefaults

Before

This discrete functionality is part of a longer function

After

Separated out

Technical Details

Part of terminally deprecating the EmailCommon class

Comments

@civibot
Copy link

civibot bot commented Aug 8, 2021

(Standard links)

@civibot civibot bot added the master label Aug 8, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the email2 branch 3 times, most recently from 0f48c1e to 99b696b Compare August 9, 2021 02:25
if (is_numeric(key($form->_fromEmails))) {
$emailID = (int) key($form->_fromEmails);
$defaults = CRM_Core_BAO_Email::getEmailSignatureDefaults($emailID);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this up seems like it would fix resetting defaults where it seems the intent was to default to the logged in contact, although it seems to do that before anyway so 🤷

This PR seems ok but I am seeing a weird thing on my local so just want to check that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy ueah I read the setting & then potentially usetting of defaults as a mistake - maybe I'm wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow where did the last 12 hours go? The patch is stale now - EmailCommon doesn't git-apply.

Otherwise if I make those changes manually it seems fine. I'll put up a PR for the thing I was seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy should I rebase or will this be superceded & perhaps closable with your patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry it should be rebased - my thing ended up being unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - rebased

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks - merge-on-pass'd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

Part of terminally deprecating the EmailCommon class
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.

2 participants