[buddypress-trac] [BuddyPress Trac] #8139: Network Invitations and Membership Requests

[buddypress-trac]

#8139: Network Invitations and Membership Requests
---------------------------+-----------------------
 Reporter:  dcavins        |       Owner:  (none)
     Type:  enhancement    |      Status:  assigned
 Priority:  normal         |   Milestone:  7.0.0
Component:  Registration   |     Version:  5.0.0
 Severity:  normal         |  Resolution:
 Keywords:  needs-refresh  |
---------------------------+-----------------------
Changes (by imath):

 * keywords:   => needs-refresh


Comment:

 Hi @dcavins,

 Thanks again for your work on this first patch. I haven't tested it yet,
 but here's a first review of the code:

 - instead of `_e()` let's use `esc_html_e()`
 - missing docblock over `do_action(
 'bp_admin_settings_after_network_invitations' );`
 - about the 'network' term, I'd suggest to simply use `invitations` or
 `relationship_invitations` instead (to avoid confusion with WordPress use
 for sites network).
 - Invitations are not a component, so I'd probably rename this function
 `bp_is_network_invitations_component()` in favor of
 `bp_is_network_invitations_screen()`
 - the @since keyword of the `bp_members_user_can_filter()` function should
 be 7.0.0 😅
 - Still in `bp_members_user_can_filter()` function what to expect about
 the `// @TODO:` comment ?
 - Looks like the `maybe_prevent_activation_emails()` function is not
 finished, was it used for your tests, maybe you should remove it?
 - the `maybe_make_registration_email_input_readonly()` misses a BP prefix
 eg: `bp_members_...`
 - About `bp_network_invitations_add_legacy_welcome_message()`,
 `bp_network_invitations_add_legacy_registration_disabled_message()`,
 `bp_network_invitations_filter_nouveau_registration_messages( $messages )`
 , I believe these hooks/functions should be placed into the `buddypress-
 functions.php` of the corresponding template packs.
 - About
 `bp_network_invitations_add_legacy_registration_disabled_message()` the
 variable `$message` should be escaped before output.
 - In `bp_network_invite_user()`, `bp_network_invitation_delete_invites()`
 you don't need to prefix the hook name when using `bp_parse_args()` as
 this function adds it. You can directly use `network_invite_user` for
 instance.
 - In `bp_network_invitation_resend_by_id()` I believe you should remove
 `error_log( 'send by id '.print_r( $success, true ) );`
 - In `bp_network_invitation_delete_invites()` you should remove extra
 lines before the closing `}` of the function.
 - I'm curious In `bp_network_invitation_get_hash()` why using
 `hash_hmac()` or `wp_generate_password()`? `wp_hash()` is problematic ? As
 it's possible to filter the hash value, I'd make things easier using
 `wp_hash()`.
 - In `bp_invitations_setup_nav()` could you align the array parameters
 like WP Code standards advise it ? For instance
 {{{
 bp_core_new_nav_item(
     array(
         'name'                    => __( 'Invitations', 'buddypress' ),
         'slug'                    => bp_get_members_invitations_slug(),
         'position'                => 80,
         'screen_function'         => 'members_screen_send_invites',
         'default_subnav_slug'     => 'list-invites',
         'show_for_displayed_user' => $user_has_access
     )
 );
 }}}
 - I'd probably move/rename this file `src/bp-members/bp-members-
 notifications.php` into the BP Groups component to avoid fatals if the
 Groups component is not active.
 - Ah maybe that was your idea when you wrote the two comments `//
 add_action( 'groups_membership_accepted',
 'groups_notification_membership_request_completed', 10, 3 );` and `//
 add_action( 'groups_membership_rejected',
 'groups_notification_membership_request_completed', 10, 3 );`
 - In `bp_members_format_notifications()` you'll need to review the
 `@since` docblock keywords and the name of the hooks.
 - What about these commented lines `// add_action(
 'groups_membership_accepted',
 'bp_groups_accept_request_mark_notifications', 10, 2 );` and `//
 add_action( 'groups_membership_rejected',
 'bp_groups_accept_request_mark_notifications', 10, 2 );`
 - In `bp_members_screen_notification_settings()` you'll need to review the
 `@since` docblock keywords and use `esc_html_e() instead of `_e()` and
 `echo esc_html_x()` instead of `_ex()`.
 - In `bp_members_invitations_slug()`, `bp_get_members_invitations_slug()`,
 `bp_has_network_invitations()`, `bp_the_network_invitations()`,
 `bp_network_invitations_pagination_count` (the filter),
 `bp_get_network_invitations_pagination_links()`,
 `bp_get_the_network_invitation_action_links` (the filter),  you'll need to
 review the `@since` docblock keyword.
 - Don't forget you still have things `// @todo` in `public function
 network_invitations_admin_load()`, `public function invitations_admin()`
 and in `public function run_send_action()` 😉
 - In `public function run_send_action()` Let's make sure multiline
 functions are written like this (WPCS) :
 {{{
 $invite_url = esc_url(
     add_query_arg(
         array(
             'inv' => $invitation->id,
             'ih'  => bp_network_invitation_get_hash( $invitation ),
         ),
         bp_get_signup_page()
     )
 );
 }}}

 btw, I'd probably do the `esc_url` into the `'invites.url'` token value.
 - In `public function run_acceptance_action()` you should review the
 `@since` docblock keywords in hooks + avoid extra empty lines at the end
 of the function.
 - In `src/bp-members/classes/class-bp-network-invitations-list-table.php`,
 you should review all `@since` docblock keywords of the file and make sure
 to align variables, and array parameters according to WPCS.
 - In `src/bp-members/classes/class-bp-network-invitations-template.php`,
 you should review all `@since` docblock keywords of the file and make sure
 to align array parameters according to WPCS.
 - In `src/bp-members/screens/list-invites.php`, `src/bp-members/screens
 /send-invites.php` & `src/bp-members/screens/send-invites.php, you should
 review all `@since` docblock keywords and avoid extra spaces into the 3
 code lines under `// Get the action.` (both functions) and where you use
 `bp_core_add_message()` without the `error` type.
 - In `src/bp-templates/bp-
 legacy/buddypress/members/single/invitations.php` & `src/bp-templates/bp-
 legacy/buddypress/members/single/invitations/invitations-loop.php` the
 `@version` docblock keyword should be `7.0.0`,
 - In `src/bp-templates/bp-legacy/buddypress/members/single/invitations
 /invitations-loop.php`, please use `esc_html_e() instead of `_e()` and be
 careful to not forget the `// @TODO`
 - About `bp_the_network_invitation_property()` I believe I would remove
 this function and create specific template tags to be able to
 sanitize/format them when needed, for instance:
 {{{
 function bp_network_invitation_content() {
     $content = bp_get_network_invitation_property( 'content' );

     echo wptexturize( $content );
 }
 }}}
 - In `src/bp-templates/bp-legacy/buddypress/members/single/invitations
 /list-invites.php`, `src/bp-templates/bp-
 legacy/buddypress/members/single/invitations/send-invites.php` & `src/bp-
 templates/bp-nouveau/buddypress/members/single/invitations.php` the
 `@version` docblock keyword should be `7.0.0` + please use `esc_html_e()
 instead of `_e()`
 - In `src/bp-templates/bp-
 nouveau/buddypress/members/single/invitations.php` you should remove `eh?`

 I'll test the patch to see how it works asap 😉

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/8139#comment:15>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac