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

[ready-for-core-team-review] Add settings to configure recently viewed items stack (Recent.php) #7586

Merged
merged 9 commits into from May 22, 2016

Conversation

@nielosz
Copy link
Contributor

nielosz commented Jan 12, 2016

This changeset introduces two new settings:

  1. recentItemsMaxCount – Size of "Recent Items" stack:
    The maximum size of the stack may be configured here. It accepts integers between 1 and 100 and defaults to 20. The formerly used constant in CRM/Utils/Recent.php has been removed.
  2. recentItemsProviders – Recent Items Providers:
    More or less all of Civi's components write to the stack. This may lead to useless views when displaying the list contents. For instance, generating pdf letters for multiple contacts may flood the list with entries which are hard to differentiate.
    This setting accepts strings representing the components and which are allowed to write to the stack, for example 'Contacts', 'Activities' or 'Grants'. If the setting is empty or unset, all components are permitted.

Both settings are located in the Administer -> System Settings -> Miscellanous.

@civicrm-builder

This comment has been minimized.

Copy link

civicrm-builder commented Jan 12, 2016

Can one of the admins verify this patch?

@totten

This comment has been minimized.

Copy link
Member

totten commented Jan 12, 2016

jenkins, ok to test

@totten

This comment has been minimized.

Copy link
Member

totten commented Jan 12, 2016

@nielosz , thanks for submitting.

Right now, the tests are failing because there are some deviations from Civi's code style. Clicking the red X in Github leads eventually to: https://test.civicrm.org/job/CiviCRM-Core-PR/7457/checkstyleResult/new/

If you'd like to run the style check locally, see http://wiki.civicrm.org/confluence/display/CRMDOC/Testing#Testing-Code-styletests and https://github.com/civicrm/civicrm-buildkit/

If you push an update to the branch, it will rerun the tests.

@yashodha yashodha added the master label Jan 13, 2016
@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Jan 13, 2016

Jesus! Okay, I'll have a look, thanks!

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Jan 14, 2016

Committed fixes

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Jan 15, 2016

Anybody reviewing this one?

@totten

This comment has been minimized.

Copy link
Member

totten commented Jan 18, 2016

Thanks for the fixes. We'll try to test it this week.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Jan 19, 2016

Just a quick note that it's worth also adding a JIRA ticket & linking the 2 https://issues.civicrm.org/jira - within the core team the assignment of work is done within JIRA - e.g the QA on this

(they try to assign things to us too ....)

@eileenmcnaughton eileenmcnaughton changed the title Add settings to configure recently viewed items stack (Recent.php) [ready-for-core-team-review] Add settings to configure recently viewed items stack (Recent.php) Jan 19, 2016
@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Jan 19, 2016

NB I also edited the title - it can be changed back when merging

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Jan 19, 2016

Thanks Eileen,
corresponding JIRA ticket is https://issues.civicrm.org/jira/browse/CRM-17838

'Pledge' => ts('Pledges'),
'Event' => ts('Events'),
'Campaign' => ts('Campaigns'),
);

This comment has been minimized.

Copy link
@colemanw

colemanw Jan 19, 2016

Member

I wonder if this list could be (partly) fetched by looking up $config->enabledComponents
Could merge that array with things like contact, relationship, activity & notes which are not components.

This comment has been minimized.

Copy link
@nielosz

nielosz Jan 19, 2016

Author Contributor

Yeah, I did this first, but i removed it for different reasons:

  1. Syncing: it's somehow different to keep in sync and en-/disabling components will cause problems: If you disabled a component A, head over to this setting, select some enabled components B and C and reenable component A, you'll get no items of this one by default. And if your users don't miss it, they will never know it would be possible.
  2. Overkill: The setting is being applied in Recent.php's add method and it's type param is used. Components names differ from this type name. It's possible to map them, but regarding 1. I thought it would be too much thrill.

But if you want, I'll do...

This comment has been minimized.

Copy link
@nielosz

nielosz Feb 8, 2016

Author Contributor

Do you want me to implement this?

This comment has been minimized.

Copy link
@colemanw

colemanw Feb 8, 2016

Member

Don't worry about it.

@colemanw

This comment has been minimized.

Copy link
Member

colemanw commented Feb 8, 2016

This looks good overall and about ready to merge. One more suggestion would be to change the deprecated CRM_Core_BAO_Setting::getItem() to Civi::settings()->get()

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Feb 18, 2016

Just missed your comment. Will do this later today...

@totten

This comment has been minimized.

Copy link
Member

totten commented Feb 18, 2016

On a design/UX/policy level, +1 -- it would make sense to list recently-viewed mailings.

On an implementation level, it may involve a bit more work than other entities:

  • The URL/page for most records is pretty stable over time. (eg contacts use civicrm/contact/view?cid=123). However, for mailings, there are different screens depending on the status (e.g. draft mailing => composition screen; sent mailing => delivery report).
  • The email composition screen (civicrm/a/#/mailing/:id aka crmMailing in AngularJS) is written as a client-side app using AJAX/CRUD APIs. To register a new item from client-side, we'd need a new AJAX API.
@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Feb 19, 2016

Yes, I know it's an angular app. After a quick glance at the code I wondered if we couldn't utilize CRM.api3('Recent', 'create') in EditMailingCtrl or crmMailingMgr?

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Apr 18, 2016

Is this one going to be merged?

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

eileenmcnaughton commented Apr 19, 2016

I think it didn't get included in this month's pr round based on who signed up for what civicrm/release-management#1

I think next month Tim will do another invite to see who can help with QA & then depending on the turn out tasks will be taken on

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented Apr 19, 2016

@omarabuhussein

This comment has been minimized.

Copy link
Member

omarabuhussein commented May 19, 2016

I've Tested this by playing with (Size of "Recent Items" ) and (Recent Items Providers) and trying different values and everything is working fine .

But there is one wrong thing .. (Size of "Recent Items" ) fields accept any value ( any number larger that 100 or less than 1 ... Any character .. .. literally anything ) and while the code treat any values not between 1 & 100 as 20 .. this is not good , some kind of validation should be there preventing anyone from inserting any number not in 1-100 range ... (Use QuickForm class formRule method , (check how other fields do it ) ) ...

Also I think 1-100 is a big range for recent items list , I think 1-15 is enough and the default should be 7 ( just an opinion ) .

Also I've just checked the code and left some notes ... Thanks

'title' => 'Size of "Recent Items" stack',
'is_domain' => 1,
'is_contact' => 0,
'description' => 'How many items should CiviCRM store in it\'s "Recently viewed" list.',

This comment has been minimized.

Copy link
@omarabuhussein

omarabuhussein May 19, 2016

Member

I think you should Add the allowed range in the description

This comment has been minimized.

Copy link
@nielosz

nielosz May 20, 2016

Author Contributor

Okay, I'll check the formRile stuff.
Or should we change type to a select and add s/th like

    'pseudoconstant' => array(
       'callback' => 'CRM_Utils_Recent::getStackSizes',
    ),

* Initialize this class and set the static variables.
*/
public static function initialize() {
$maxItemsSetting = Civi::settings()->get('recentItemsMaxCount');
if (isset($maxItemsSetting) && $maxItemsSetting > 0 && $maxItemsSetting < 100) {

This comment has been minimized.

Copy link
@omarabuhussein

omarabuhussein May 19, 2016

Member

So if you don't allow anyone to insert value not in 1-100 range , then this checking part is not necessary and you can just keep isset($maxItemsSetting)

This comment has been minimized.

Copy link
@nielosz

nielosz May 20, 2016

Author Contributor

For this is a db setting, I tend to leave it double checked.

*
* @param string $providerName
*/
public static function isProviderEnabled($providerName) {

This comment has been minimized.

Copy link
@omarabuhussein

omarabuhussein May 19, 2016

Member

Do you think this method is necessary to be public , Could you see that it can be useful and may be used outside the context of this class by you or someone else ... If not then please make it private because I don't see a point for it to be public in this case .

This comment has been minimized.

Copy link
@nielosz

nielosz May 20, 2016

Author Contributor

It's public because providers may check it before adding items to the stack. But you're right at the moment this isn't used anymore since I utilized the $type param.

This comment has been minimized.

Copy link
@omarabuhussein

omarabuhussein May 20, 2016

Member

oh I see ... don't worry about it then .

@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented May 20, 2016

You wrote

But there is one wrong thing .. (Size of "Recent Items" ) fields accept any value

Regarding form validation: shouldn't a setting defined as "Integer" only accept integers?

nielosz added 2 commits Jan 12, 2016
…k: 1.) Define stack size, 2.) Decide which components may store items in stack
@nielosz nielosz force-pushed the nielosz:configure-recent branch from 8ff217f to 074585b May 20, 2016
nielosz added 2 commits May 20, 2016
@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented May 20, 2016

@omarabuhussein I did reflect your comments. Please give it a try.

@totten

This comment has been minimized.

Copy link
Member

totten commented May 21, 2016

jenkins, test this please

@omarabuhussein

This comment has been minimized.

Copy link
Member

omarabuhussein commented May 22, 2016

Or should we change type to a select and add s/th like

No I think your current way is better.

Regarding form validation: shouldn't a setting defined as "Integer" only accept integers?

to be honest I don't know, I have to investigate more into the code to know more about that ... But it appear that it didn't work that way ..


Anyway I've checked the changes in the code and tested it again and it appear that it work without any issue now ... But since the review week is ended I am not sure if you guys can merge it (cc : @totten )

@totten

This comment has been minimized.

Copy link
Member

totten commented May 22, 2016

Yeah, Review Week technically ended a few days ago, and 4.7.8 RC was published yesterday (branch 4.7.8-rc and tarballs). OTOH, it sounds like this has been vetted, and it would be overly bureaucratic to put this on hold and then another round of review.

I'll go ahead and merge into master, which means it will be published in the next round (4.7.9) but doesn't need any other review.

@totten totten merged commit 4fd7786 into civicrm:master May 22, 2016
1 check passed
1 check passed
default Merged build finished.
Details
@nielosz

This comment has been minimized.

Copy link
Contributor Author

nielosz commented May 22, 2016

nicy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.