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#2096 Fix in-master-only regression on creating new events #18710

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 8, 2020

Overview

unreleased regression - master branch - Creating a new event without an email produces Mandatory values missing from Api4 Email::save: email failure

Reproduction steps

  1. Click on Events -> New Event.
  2. Enter mandatory fields Event Type, Default Role, Event Title and Start and click Continue.
  3. Observe Email is not required
  4. Click Save and Done

Before

Error: Mandatory values missing from Api4 Email::save: email

After

No error!

@civibot
Copy link

civibot bot commented Oct 8, 2020

(Standard links)

@aydun
Copy link
Contributor

aydun commented Oct 8, 2020

  • General standards
    • (r-explain) Pass
    • (r-user) Pass:
    • (r-doc) Pass
    • (r-run) Pass: Tested on build test site. Scenario in ticket is fixed. But found another bug relating to address (will document shortly)
  • Developer standards

@totten
Copy link
Member

totten commented Oct 8, 2020

Thanks @aydun!

(r-run) Pass: Tested on build test site. Scenario in ticket is fixed. But found another bug relating to address (will document shortly)

Cool.

(r-test) Undecided Someone once said tests were a good idea :-)

:)

I'll merge it - we're supposed to branch for 5.31-rc right... about... now... Since this is a very-recent (same-version) regression, it's easier if we get it in before branching.

Aside: I also did a bit of review. The only bit to add -- I grepped a bit on the affected class, and it only appears to affect this one use-case ("Edit Event"), so not concerned about side-effects. ✔️

@totten totten merged commit c745957 into civicrm:master Oct 8, 2020
@eileenmcnaughton eileenmcnaughton deleted the locev2 branch October 8, 2020 09:16
@aydun
Copy link
Contributor

aydun commented Oct 8, 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.

3 participants