Skip to:
Content

BuddyPress.org

Opened 3 weeks ago

Closed 3 weeks ago

#8444 closed enhancement (fixed)

BP_Invitation: Increment date_modified on key actions.

Reported by: dcavins Owned by: dcavins
Milestone: 8.0.0 Priority: normal
Severity: normal Version: 7.2.0
Component: Core Keywords: has-patch commit
Cc:

Description

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.

Attachments (3)

8444.1.diff (11.0 KB) - added by dcavins 3 weeks ago.
Add invitation date_modified changes and tests.
8444.patch (11.0 KB) - added by imath 3 weeks ago.
8444.2.diff (13.3 KB) - added by dcavins 3 weeks ago.
Add invitation date_modified changes and tests. Improve inline documentation.

Download all attachments as: .zip

Change History (9)

@dcavins
3 weeks ago

Add invitation date_modified changes and tests.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


3 weeks ago

@imath
3 weeks ago

#2 @imath
3 weeks ago

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 ?

#3 @dcavins
3 weeks ago

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

#4 @dcavins
3 weeks ago

@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
3 weeks ago

Add invitation date_modified changes and tests. Improve inline documentation.

#5 @imath
3 weeks ago

  • Keywords commit added

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

#6 @dcavins
3 weeks ago

  • Owner set to dcavins
  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.