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

Fix notices on event settings form #13027

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Fix notices on event settings form #13027

merged 1 commit into from
Oct 30, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Oct 29, 2018 •
edited
Loading

Overview

Per #13023 fix recent spawning of e-notice on preferences forms by pushing through to cleaning them up fully

Before

screenshot 2018-10-29 20 58 16

After

screenshot 2018-10-29 20 52 58

Technical Details

details in #13023 but note that I altered the handing of 'Dashboard entries' as it incorrectly made it seem optional. The setting is used to determine how many events to show on the events dashboard.

Comments

@civibot
Copy link

civibot bot commented Oct 29, 2018

(Standard links)

@civibot civibot bot added the master label Oct 29, 2018
@mattwire
Copy link
Contributor

@eileenmcnaughton checkstyle and conflicting file

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I've fixed but I will rebase again once #13026 is merged

@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@seamuslee001
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 OMG OMG - tests finally got through on this!

seamuslee001
seamuslee001 reviewed Oct 30, 2018
View reviewed changes
CRM/Core/SelectValues.php
*/
public static function getDashboardEntriesCount() {
$optionValues = [];
$optionValues[-1] = ts('show all');
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton why not just empty value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So given I needed to key by -1 for later handling then I couldn't get it to pass style warnings by just declaring the array - it wanted a space between the minus and the 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(using -1 is a bit dumb but that is what is currently handled when this setting is used)

@seamuslee001
Copy link
Contributor

Its strange having show all as a negative number but i checked the usage in CRM/Event/BAO/Event.php and confirmed that negative numbers are expected to equal show all.

(CiviCRM Review Template WORD-1.1)

@seamuslee001
Copy link
Contributor

Merging

@seamuslee001 seamuslee001 merged commit 880f730 into civicrm:master Oct 30, 2018
@seamuslee001 seamuslee001 deleted the settings_event branch October 30, 2018 21:10
@eileenmcnaughton
Copy link
Contributor Author

thanks @seamuslee001

@eileenmcnaughton eileenmcnaughton mentioned this pull request Oct 31, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @mattwire @seamuslee001