BP_Invitation: Increment date_modified on key actions.

[dcavins]

When an invitation is sent or accepted, the date_modified should be updated.
However, if a date_modified has been specified in the invitation or request
creation function (as we do in our unit test functions), the date_modified should be set to that, even if notices are sent (now) as part of the creation process.
Ensure that BP_Invitation::get_query_clauses() handles specified date_modified data updates.

[dcavins]

Add invitation date_modified changes and tests.

[imath]

Hi @dcavins

I've just tested the patch, using the unit tests. It seems fine. Thanks a lot for your work on this.

Some questions/advices ☺️

1. At line 142 of src/bp-core/classes/class-bp-invitation-manager.php why aren't you using the $r parsed arguments instead of the $args unparsed ones? I'd expect something like: $sent = $this->send_invitation_by_id( $invite_id, $r );
2. Could you describe the arguments of the array in the docblock of BP_Invitation_Manager->send_invitation_by_id() and in BP_Invitation_Manager->send_request_notification_by_id() ? (​WP documentation standards for arrays)
3. About lines 492 & 538 of src/bp-core/classes/class-bp-invitation-manager.php why can't we just use $this->mark_accepted( $r ) the modified date should be in it as $r is a parsed version of $args ?
4. In src/bp-core/classes/class-bp-invitation.php the BP_Invitation::mark_accepted_by_data is not using the second argument, maybe we can get rid of it and do the same for BP_Invitation_Manager->mark_accepted() in src/bp-core/classes/class-bp-invitation-manager.php ?

[dcavins]

Hi @imath,

Thanks for taking the time to look at this patch. I hope you didn't have trouble applying it, though I'm guessing you did because you attached a patch that looks the same except for line numbers. :)

Regarding your questions:
1) I used the original, passed args so that a date_modified would only be included when the date_modified was explicitly passed, rather than the parsed default value. I guess the difference isn't meaningful--if we were to pass $r along, then the sent time would be the same as if no date_modified were passed at all (you'd end up with bp_core_current_time() in both cases). So, I was trying to maintain the dev's intent, but it comes out in the wash; using the parsed args seems more consistent.
2) Sure. Should I only include the arguments that currently have an effect (date_modified)?
3) This is the same as 1 above.
4) Yes that was a typo (I intended to use passed_args but didn't), but the answer to the first question applies here.

Ha, really the vast majority of this code was added to make sure that our invitation pagination tests would pass (they require you to set the date_modified to work reliably because of the too-many-operations-happen-in-second problem). But setting those dates, while useful, is probably not common.

Thanks again for your thoughts! It's simpler to think about it the way you suggest.

-David

[dcavins]

@imath, thanks again for your review. I've further simplified the patch and expanded the inline documentation in several places. I think this patch is ready to commit, if you don't have any objections.

[dcavins]

Add invitation date_modified changes and tests. Improve inline documentation.

[imath]

I agree @dcavins 👌 Thanks a lot for these improvements, feel free to commit it.

[dcavins]

In 12873:

BP_Invitation: Increment date_modified on key actions.

When an invitation is sent or accepted, the date_modified
should be updated. However, if a date_modified has been
specified in the invitation or request creation function
(as we do in our unit test functions), the date_modified
should be set to that, even if notices are sent (now) as
part of the creation process.

Ensure that BP_Invitation::get_query_clauses() handles specified date_modified data updates.

Fixes #8444.