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#1822 - Fix missing newly created activity types from the dropdown on the new activity form #17625

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Jun 15, 2020

Overview

https://lab.civicrm.org/dev/core/-/issues/1822

Adding a new activity type and then going to create a new activity doesn't show the new type in the dropdown on the form. Stopped working in 5.26.

Technical Details

The change to use the database default for the filter column changed it to null, but it needs 0. One other way to deal with this would be to make the need for 0 in the code more tolerant to also accept null to mean the same thing. Another would be to put back the code that sets a missing filter value to FALSE. But this PR seems more in line with the intended direction of the earlier change.

Comments

One thing I'm not sure about is that updating any that got inadvertently set to null will also update ones that someone might have set to null on purpose. But that seems unlikely? Should remove the code comment in the sql upgrade file if it's decided to go with this PR.

@civibot
Copy link

civibot bot commented Jun 15, 2020

(Standard links)

@civibot civibot bot added the 5.27 label Jun 15, 2020
@demeritcowboy demeritcowboy force-pushed the missing-activitytype-5.27 branch from 9cfd7c2 to b8be322 Compare June 15, 2020 23:47
@demeritcowboy demeritcowboy changed the title WIP - dev/core#1822 - Fix missing newly created activity types from the dropdown on the new activity form dev/core#1822 - Fix missing newly created activity types from the dropdown on the new activity form Jun 16, 2020
@seamuslee001
Copy link
Contributor

This seems sensible to me @colemanw ?

@colemanw
Copy link
Member

Looks good.

@demeritcowboy
Copy link
Contributor Author

Another thought which I just had: Setting the default to 0 works for activity types, but might introduce a problem somewhere for other option values if they are expecting null and don't deal with 0. I don't know of any offhand, but the default even before the earlier change was null, so for other option values where they don't explicitly do anything they'll now get 0. I'll try grepping for filter and see what comes up.

@seamuslee001
Copy link
Contributor

so the filter up and till now was being set to FALSE aka 0 if not set. So they would have had to be passing in a 'Null' value iirc to set it to NULL so I think this is a safe change to make

@demeritcowboy
Copy link
Contributor Author

Oh right it was in BAO/OptionValue - I confused it with Activity. Good because the grep wasn't looking fun.

@seamuslee001 seamuslee001 merged commit a6cda52 into civicrm:5.27 Jun 16, 2020
@demeritcowboy demeritcowboy deleted the missing-activitytype-5.27 branch June 16, 2020 00:59
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