Introduce advanced Exclude tab to replace Connectors tab #251 #278

faishal · 2014-02-25T00:01:53Z

Resolved #251

Added Exclude setting with backwards compatibility.
Restricting record being logged as per exclude setting.

…nectors and IP address #251

…able when excluded #251

westonruter · 2014-02-25T01:34:45Z

classes/connector.php

+		 * @param  string        Current class name
+		 * @return bool
+		 */
+		return apply_filters( 'wp_stream_record_log', $bool, $user, get_called_class() );


@faishal nice! even including hook docs.

@faishal Unless we have this filter somewhere else with the same name, and receiving same input, we should change the filter name to reflect what it really does.
It'd also be great if we can change the description to something like Filter to exclude actions of a specific user from being logged or something.
Nice work man! /five!

@faishal Also wanted to leave a /five for this!

…and replace exit with wp_send_json methods #278 #251

Conflicts: includes/connectors.php

#251 #278

faishal · 2014-02-25T21:28:04Z

@westonruter Thanks for your suggestion, I have fixed all the issues that you mention in comments.

Also fix issues ocure after merging develop branch

westonruter · 2014-02-25T21:49:14Z

@faishal cheers!

@fjarrett @shadyvb over to you!

shadyvb · 2014-02-25T22:28:31Z

classes/connector.php

@@ -31,6 +31,9 @@ public static function register() {
 		if ( ! self::is_logging_enabled_for_user() ) {
 			return;
 		}
+		if ( ! self::is_logging_enabled_for_ip() ) {


Adding checks in the base class of all connector will cause it to be executed by all connectors while they try to register themselves, which is something we probably don't need. We probably need to move those checks ( including is_logging_enabled_for_user by @jonathanbardo ) to includes/connectors.php so it is done once on plugin load.
@fjarrett @jonathanbardo What do you think ?

Yes I think it would be more efficient.

@faishal If you agree as well on this, please carry on with moving any redundant checks ( IP / User exclusion ) to the WP_Stream_Connectors class.

@shadyvb I am also agree with that.

@shadyvb should i move functions is_logging_enabled_for_user & is_logging_enabled_for_ip into WP_Stream_Settings because it is a part of settings

I'd prefer putting them in the same class that'll use them, just to make it easier for people dealing with the same functionality later. Unless other classes would be using them, which is a far aim in this scenario.

frankiejarrett · 2014-02-26T05:28:32Z

includes/settings.php

+					$labels
+				), $old_options [ 'connectors_active_connectors' ]
+			);
+			unset( self::$options[ 'connectors_active_connectors' ] );


@faishal Also, the space usage within brackets is incorrect throughout the code in this PR, according to WP coding standards.

self::$options['connectors_active_connectors']

"When referring to array items, only include a space around the index if it is a variable"

cf. http://make.wordpress.org/core/handbook/coding-standards/php/#space-usage

shadyvb · 2014-02-26T08:50:35Z

With all the CS comments from @fjarrett , i'm curios to know if @faishal did setup pre-commit hook with dependencies ( phpcs / jslint etc.. ). Would make it way easier for @faishal to correct stuff like that.
Note: Setup instructions should be available in https://github.com/x-team/wp-stream/blob/master/contributing.md

faishal · 2014-02-26T13:46:37Z

@shadyvb I use phpStorm, in that i have configure codesniffer for wordpress standards and also show sniffer warning as ERROR, so if i missed something then it will alert me.

I also checked it by run command manually on terminal before pushing it to master, but i not found any error in that also.

pre-commit idea is also good.

shadyvb · 2014-02-26T13:51:19Z

We do have a pre-commit file inside bin directory, you'd just need to symlink it to .git/hooks so it works properly, more info on that is found on our plugin boilerplate repo.

Also i'm not sure if this would be helpful to you, but we do have a configuration template for phpstorm that should make it easier to follow WPCS as well, check it out.

westonruter · 2014-02-26T16:10:40Z

@shadyvb The pre-commit hook wouldn't have helped because, as you see, our sniffs aren't fleshed out fully with all of the WordPress coding conventions. Travis isn't complaining here, so the pre-commit hook wouldn't complain either. So yeah, it would be good to create some new WPCS issues for enforcing the conventions we've discussed in this PR.

…es with select2 #278 #251

faishal · 2014-02-27T09:25:07Z

@shadyvb @fjarrett I have fixed all the issues that you mention in comments.

shadyvb · 2014-02-27T09:42:54Z

@faishal There seem to be some conflicts with develop, please merge and resolve them.

Great work!

Conflicts: includes/install.php

faishal · 2014-02-27T10:01:20Z

@shadyvb Thanks, I just fixed that conflicts and other issues

frankiejarrett · 2014-02-27T15:38:13Z

@faishal I've been testing this, and it's working quite nicely! There are only two issues that I can find that need to be resolved before we merge this feature:

In Disabled connectors should be removed from default queries and filter dropdowns #223 we removed references to excluded Connectors from appearing anywhere in the Stream. So now that we are able to exclude Author, Role, Context, Action and IP - the same must be done for those as well.
We must also ensure that the Live Update of the Stream does not try to bring in any previously-tracked activity that is now excluded. For instance, if I update a page and then decide to exclude the "Update" action, the live update should not let it enter the Stream because it should be excluded.

frankiejarrett · 2014-03-03T21:39:31Z

@faishal Do let me know if you have any questions regarding the two issues I mentioned above.

faishal · 2014-03-04T20:00:23Z

@fjarrett Sorry for delay, I just started looking in to it.
Regarding 1st issue, I am confuse about roles related references as we stored user_id in database and 2nd issue is already done.

frankiejarrett · 2014-03-05T05:46:23Z

@faishal You're right about the Role, doesn't look like we'll be able to easily omit records based on that. However, the others (author, context, action, IP) we should be able to completely hide from the Stream, correct?

Regarding my second point, please disregard it. I believe my testing was flawed here, I must have been tired 😄

… Private Feed #251 #278

Conflicts: includes/install.php ui/admin.css

faishal · 2014-03-05T19:59:07Z

Absolutely correct @fjarrett.

faishal · 2014-03-07T12:36:59Z

@fjarrett, is anything else left to be done in here?

frankiejarrett · 2014-03-07T15:15:21Z

@faishal I'm going to fully test this here in a few hours and I'll let you know.

faishal added 7 commits February 22, 2014 04:52

Exclude setting added #251

75f8d91

Bind select2 js to chose type field && Handle Exclude setting for con…

93f67d3

…nectors and IP address #251

Merge branch 'master' of https://github.com/x-team/wp-stream

f0055d5

Worked on exlude context and actions, Handle hide records from list t…

be7107e

…able when excluded #251

Worked on setting backwards compatiblity and jshint issues #251

6ce5757

Handle settings backwards compatiblity and User&Role exclude setting #…

0b32a07

…251

Merge branch 'develop' of https://github.com/x-team/wp-stream

53967d3

westonruter reviewed Feb 25, 2014
View reviewed changes

faishal added 3 commits February 26, 2014 01:46

Add nonce & permission check, escape issue suggested by @westonruter …

f675f64

…and replace exit with wp_send_json methods #278 #251

Merge branch 'develop' of https://github.com/x-team/wp-stream

dda9bd5

Conflicts: includes/connectors.php

Fix issue after merging develop branch occure due to connector refactor

dad30d9

#251 #278

shadyvb reviewed Feb 25, 2014
View reviewed changes

frankiejarrett reviewed Feb 26, 2014
View reviewed changes

Fix phpcs issue, move function as per context, rename chosen referenc…

73fca85

…es with select2 #278 #251

faishal added 2 commits February 27, 2014 15:22

Merge branch 'develop' of https://github.com/x-team/wp-stream

94f5fbf

Conflicts: includes/install.php

Fix undefined function issue

5d6bd93

frankiejarrett self-assigned this Feb 28, 2014

faishal added 2 commits March 6, 2014 00:50

Filter all excluded record from List table, Dashboard Widget and User…

0a487d7

… Private Feed #251 #278

Merge branch 'develop' of https://github.com/x-team/wp-stream

bcc7116

Conflicts: includes/install.php ui/admin.css

frankiejarrett merged commit bcc7116 into xwp:develop Mar 7, 2014

frankiejarrett mentioned this pull request Mar 27, 2014

Select2 for Role/User Access Field #272

Closed

faishal unassigned frankiejarrett Aug 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce advanced Exclude tab to replace Connectors tab #251 #278

Introduce advanced Exclude tab to replace Connectors tab #251 #278

faishal commented Feb 25, 2014

westonruter Feb 25, 2014

shadyvb Feb 25, 2014

frankiejarrett Feb 26, 2014

faishal commented Feb 25, 2014

westonruter commented Feb 25, 2014

shadyvb Feb 25, 2014

jonathanbardo Feb 25, 2014

shadyvb Feb 25, 2014

faishal Feb 25, 2014

faishal Feb 26, 2014

shadyvb Feb 26, 2014

frankiejarrett Feb 26, 2014

shadyvb commented Feb 26, 2014

faishal commented Feb 26, 2014

shadyvb commented Feb 26, 2014

westonruter commented Feb 26, 2014

faishal commented Feb 27, 2014

shadyvb commented Feb 27, 2014

faishal commented Feb 27, 2014

frankiejarrett commented Feb 27, 2014

frankiejarrett commented Mar 3, 2014

faishal commented Mar 4, 2014

frankiejarrett commented Mar 5, 2014

faishal commented Mar 5, 2014

faishal commented Mar 7, 2014

frankiejarrett commented Mar 7, 2014

Introduce advanced Exclude tab to replace Connectors tab #251 #278

Introduce advanced Exclude tab to replace Connectors tab #251 #278

Conversation

faishal commented Feb 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faishal commented Feb 25, 2014

westonruter commented Feb 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadyvb commented Feb 26, 2014

faishal commented Feb 26, 2014

shadyvb commented Feb 26, 2014

westonruter commented Feb 26, 2014

faishal commented Feb 27, 2014

shadyvb commented Feb 27, 2014

faishal commented Feb 27, 2014

frankiejarrett commented Feb 27, 2014

frankiejarrett commented Mar 3, 2014

faishal commented Mar 4, 2014

frankiejarrett commented Mar 5, 2014

faishal commented Mar 5, 2014

faishal commented Mar 7, 2014

frankiejarrett commented Mar 7, 2014