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/485 Website type should be required as other type values are #13170

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

elisseck
Copy link
Contributor

Overview

Strange behavior possible as described in https://lab.civicrm.org/dev/core/issues/485.

A user could save a website record without a 'website type' (applies to any contact type, not just organization as in the ticket). Storing this value without a type, which should be required, causes data to be seemingly mysteriously deleted.

Before

User is able to click the (x) and save a website record with no type
image

After

User can no longer click an (x) and must instead choose a website type, even if they create their own option value labelled "none", they must choose a type.
image

Technical Details

Adding the 'placeholder' => NULL option here is what looks standard when looking at the code for IM, Phone, Email, etc. They all require a type to be saved, so website should, too... or we'll run into trouble.

@civibot
Copy link

civibot bot commented Nov 29, 2018

(Standard links)

@civibot civibot bot added the master label Nov 29, 2018
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 @colemanw @mattwire @lcdservices do you have an opinion on this - it is a behaviour change but the previous behaviour was a bit broken....I guess what might happen is people have lots of website in their db without type & every time they go to edit them they get forced to add type....

Having said that - I would assume that without type you can't export them etc so it is kinda invalid

@lcdservices
Copy link
Contributor

I agree that makes sense and is consistent with the other data objects.

@yashodha
Copy link
Contributor

works fine and is consistent with others.
@lcdservices @eileenmcnaughton shall we merge this?

@eileenmcnaughton
Copy link
Contributor

Thanks for the ping @yashodha - looks like you and @lcdservices have weighed in in favour of merging this so lets do it!

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.

4 participants