| @@ -25,8 +25,28 @@ | ||
| 25 | 25 | class Forms_Data { |
| 26 | 26 | use Get_Instance; |
| 27 | 27 | |
| 28 | 28 | /** |
| 29 | + * Ids of the in-window submitters who can edit the site, keyed by blog id. | |
| 30 | + * | |
| 31 | + * Keyed on the blog rather than held as a single list because capabilities are | |
| 32 | + * per-site: after a switch_to_blog() the previous site's answer is wrong, and a | |
| 33 | + * flat cache would hand it back. | |
| 34 | + * | |
| 35 | + * A class property rather than a `static` inside the method so it can be reset, | |
| 36 | + * which the tests need: they create a user and then ask for the metrics inside | |
| 37 | + * one process. | |
| 38 | + * | |
| 39 | + * Deliberately per-request and never written to the object cache. A persistent | |
| 40 | + * cache would keep counting a newly promoted editor as a visitor until | |
| 41 | + * something invalidated it, and there is no natural invalidation point. | |
| 42 | + * | |
| 43 | + * @var array<int,array<int,int>>|null | |
| 44 | + * @since 2.12.7 | |
| 45 | + */ | |
| 46 | + private static $editing_user_ids = null; | |
| 47 | + | |
| 48 | + /** | |
| 29 | 49 | * Constructor |
| 30 | 50 | * |
| 31 | 51 | * @since 0.0.1 |
| 32 | 52 | */ |
| @@ -306,9 +326,14 @@ | ||
| 306 | 326 | * @since 2.12.6 |
| 307 | 327 | */ |
| 308 | 328 | $limit = Helper::get_integer_value( apply_filters( 'srfm_forms_metric_sort_limit', 500 ) ); |
| 309 | 329 | |
| 310 | - if ( $limit > 0 && count( $id_query->posts ) > $limit ) { | |
| 330 | + // A filter is allowed to tighten or loosen the ceiling, not to remove it. Zero | |
| 331 | + // or negative read as "no ceiling" to a `> $limit` test, which is the one | |
| 332 | + // outcome the ceiling exists to prevent, so fall back to the default. | |
| 333 | + $limit = $limit > 0 ? $limit : 500; | |
| 334 | + | |
| 335 | + if ( count( $id_query->posts ) > $limit ) { | |
| 311 | 336 | /** |
| 312 | 337 | * Fires when the metric sort is skipped because the site has too many forms. |
| 313 | 338 | * |
| 314 | 339 | * Announced rather than skipped silently: a list that quietly ignores the |
| @@ -347,22 +372,14 @@ | ||
| 347 | 372 | $rows = []; |
| 348 | 373 | foreach ( $id_query->posts as $post_id ) { |
| 349 | 374 | $form_id = Helper::get_integer_value( $post_id ); |
| 350 | 375 | |
| 351 | - // Same three arguments the render path passes. Calling this with only the | |
| 352 | - // form ID left $post_date_gmt empty, so strtotime() returned false, the | |
| 353 | - // "form is younger than the window" shortcut could never be taken, and the | |
| 354 | - // windowed COUNT ran for every row — both a second query per form and, for | |
| 355 | - // any entry whose created_at predates the form's post_date (an import, a | |
| 356 | - // migration, a restored backup), a different number than the column shows. | |
| 357 | - // The comment below promises order and display can never disagree; passing | |
| 358 | - // different arguments here is what made them disagree. | |
| 359 | - $post = get_post( $form_id ); | |
| 360 | - $metrics = $this->calculate_form_metrics( | |
| 361 | - $form_id, | |
| 362 | - $post->post_date_gmt ?? '', | |
| 363 | - Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ) | |
| 364 | - ); | |
| 376 | + // The form id is the whole input, here and on the render path, so the two | |
| 377 | + // cannot be handed different arguments and compute different numbers. | |
| 378 | + // They once could: this took a creation date and an all-time count as | |
| 379 | + // well, the two callers passed them differently, and the column and its | |
| 380 | + // sort order disagreed. | |
| 381 | + $metrics = $this->calculate_form_metrics( $form_id ); | |
| 365 | 382 | |
| 366 | 383 | // Same helper the column renders from, so the order always matches the |
| 367 | 384 | // numbers on screen. An unmeasurable rate sorts as -1 rather than 0, so |
| 368 | 385 | // the dash rows group below a genuine 0% instead of tying with it. |
| @@ -443,8 +460,146 @@ | ||
| 443 | 460 | return gmdate( 'Y-m-d H:i:s', $window_start + $offset_seconds ); |
| 444 | 461 | } |
| 445 | 462 | |
| 446 | 463 | /** |
| 464 | + * Ids of the in-window submitters who can edit the site, cached for the request. | |
| 465 | + * | |
| 466 | + * Bounded by submitters, not by users. Asking the user table for everyone with | |
| 467 | + * `edit_posts` runs an unindexable leading-wildcard scan of capability meta with | |
| 468 | + * no LIMIT, and the answer is then interpolated into one windowed COUNT per form, | |
| 469 | + * up to the ceiling in get_forms_sorted_by_metric(). On a membership site that | |
| 470 | + * hands `contributor` to every member that is a five-figure placeholder list | |
| 471 | + * rebuilt for every row of a ten-row page. Only people who actually submitted | |
| 472 | + * inside the window can affect the rate, and that set is small. | |
| 473 | + * | |
| 474 | + * Served by `idx_user_id_created_at (user_id, created_at)`, added for this | |
| 475 | + * lookup. `idx_user_id` alone cannot: `created_at` is not in it and no other | |
| 476 | + * index leads on `created_at`, so before that index this was a range scan with | |
| 477 | + * a row read per row plus a temp table for the DISTINCT -- on a site with years | |
| 478 | + * of logged-in submissions, every entry ever recorded, to return a short list. | |
| 479 | + * The LIMIT bounds what comes back, not what is read; the index bounds the | |
| 480 | + * read. | |
| 481 | + * | |
| 482 | + * Decided with `user_can()`, the same call Form_Views::should_track() makes, so | |
| 483 | + * both halves of the rate answer one question rather than two similar ones. A | |
| 484 | + * capability granted at runtime through `user_has_cap`, a multisite super admin | |
| 485 | + * who is not a member of the subsite, and a role carrying `edit_posts` as an | |
| 486 | + * explicit denial all resolve the same way on both sides. | |
| 487 | + * | |
| 488 | + * Reflects capability as it stands now, not as it stood at submission time. On a | |
| 489 | + * site that promotes or demotes people the two halves still drift: a promoted | |
| 490 | + * subscriber's earlier views stay in the denominator while their earlier entries | |
| 491 | + * leave the numerator, and a demoted editor's earlier entries return without | |
| 492 | + * their views. Closing that means stamping the decision on the entry at submit | |
| 493 | + * time, which is a schema change this percentage does not justify. | |
| 494 | + * | |
| 495 | + * Cached because the forms listing asks once per row, and the answer cannot | |
| 496 | + * change within a request. | |
| 497 | + * | |
| 498 | + * @param int $window_start Unix timestamp the view window opened at. | |
| 499 | + * @since 2.12.7 | |
| 500 | + * @return array<int,int> | |
| 501 | + */ | |
| 502 | + private static function get_editing_submitter_ids( $window_start ) { | |
| 503 | + $blog_id = get_current_blog_id(); | |
| 504 | + | |
| 505 | + if ( null === self::$editing_user_ids ) { | |
| 506 | + self::$editing_user_ids = []; | |
| 507 | + } | |
| 508 | + | |
| 509 | + if ( isset( self::$editing_user_ids[ $blog_id ] ) ) { | |
| 510 | + return self::$editing_user_ids[ $blog_id ]; | |
| 511 | + } | |
| 512 | + | |
| 513 | + /** | |
| 514 | + * Largest number of distinct in-window submitters to test for edit access. | |
| 515 | + * | |
| 516 | + * The test is bounded work per submitter, so this is a guard against a site | |
| 517 | + * where a very large share of submissions are made while logged in. Past the | |
| 518 | + * ceiling the exclusion is skipped rather than truncated: a partial exclusion | |
| 519 | + * list reports a rate that is wrong in a way nobody can see, where no | |
| 520 | + * exclusion at least reproduces the pre-existing behaviour. | |
| 521 | + * | |
| 522 | + * Two things to know before raising or lowering it. The count is of | |
| 523 | + * distinct submitters across all forms since tracking was first enabled, | |
| 524 | + * and that window never resets -- so a membership site, a store or an LMS | |
| 525 | + * reaches 500 in ordinary operation, and once passed it stays passed, with | |
| 526 | + * the rate quietly counting editor submissions again. That is why the | |
| 527 | + * settings copy says those are "normally" left out rather than promising it | |
| 528 | + * outright. And the query does not filter on status, so a user whose only | |
| 529 | + * in-window entries were trashed still lands on the list and consumes | |
| 530 | + * budget -- harmless for the count, but it brings the ceiling closer. | |
| 531 | + * | |
| 532 | + * @param int $limit Maximum submitters to test. Default 500. | |
| 533 | + * @since 2.12.7 | |
| 534 | + */ | |
| 535 | + $limit = Helper::get_integer_value( apply_filters( 'srfm_forms_metric_submitter_limit', 500 ) ); | |
| 536 | + | |
| 537 | + // Same reasoning as the sort ceiling: a filter may move it, not remove it. | |
| 538 | + $limit = $limit > 0 ? $limit : 500; | |
| 539 | + | |
| 540 | + // One row past the ceiling, so a full page is proof the ceiling was passed | |
| 541 | + // without counting the rest of the table to find out. | |
| 542 | + $rows = Entries::get_instance()->get_results( | |
| 543 | + [ | |
| 544 | + [ | |
| 545 | + [ | |
| 546 | + 'key' => 'created_at', | |
| 547 | + 'compare' => '>=', | |
| 548 | + 'value' => self::window_boundary_sql( $window_start ), | |
| 549 | + ], | |
| 550 | + [ | |
| 551 | + 'key' => 'user_id', | |
| 552 | + 'compare' => '>', | |
| 553 | + 'value' => 0, | |
| 554 | + ], | |
| 555 | + ], | |
| 556 | + ], | |
| 557 | + 'DISTINCT user_id', | |
| 558 | + [ sprintf( 'LIMIT %d', $limit + 1 ) ] | |
| 559 | + ); | |
| 560 | + | |
| 561 | + $submitters = array_values( array_unique( array_map( 'absint', array_column( $rows, 'user_id' ) ) ) ); | |
| 562 | + | |
| 563 | + if ( count( $submitters ) > $limit ) { | |
| 564 | + /** | |
| 565 | + * Fires when the editor exclusion is skipped because too many submitters | |
| 566 | + * would have to be tested. | |
| 567 | + * | |
| 568 | + * Announced rather than skipped silently, for the same reason as | |
| 569 | + * `srfm_forms_metric_sort_skipped`: a rate that quietly stops excluding | |
| 570 | + * editors reads as a wrong number, not as a deliberate ceiling. | |
| 571 | + * | |
| 572 | + * @param int $count Number of distinct submitters found, capped at $limit + 1. | |
| 573 | + * @param int $limit The ceiling in force. | |
| 574 | + * @since 2.12.7 | |
| 575 | + */ | |
| 576 | + do_action( 'srfm_forms_metric_submitter_limit_exceeded', count( $submitters ), $limit ); | |
| 577 | + | |
| 578 | + self::$editing_user_ids[ $blog_id ] = []; | |
| 579 | + | |
| 580 | + return self::$editing_user_ids[ $blog_id ]; | |
| 581 | + } | |
| 582 | + | |
| 583 | + if ( [] !== $submitters ) { | |
| 584 | + // Two queries for the whole set. Without it user_can() resolves each user | |
| 585 | + // on its own and the loop becomes one query per submitter. | |
| 586 | + cache_users( $submitters ); | |
| 587 | + } | |
| 588 | + | |
| 589 | + self::$editing_user_ids[ $blog_id ] = array_values( | |
| 590 | + array_filter( | |
| 591 | + $submitters, | |
| 592 | + static function ( $user_id ) { | |
| 593 | + return user_can( $user_id, 'edit_posts' ); | |
| 594 | + } | |
| 595 | + ) | |
| 596 | + ); | |
| 597 | + | |
| 598 | + return self::$editing_user_ids[ $blog_id ]; | |
| 599 | + } | |
| 600 | + | |
| 601 | + /** | |
| 447 | 602 | * Views and conversion rate for one form. |
| 448 | 603 | * |
| 449 | 604 | * The single source of truth for both the rendered value and the sorted metric. |
| 450 | 605 | * They were computed separately at first, and drifted: the sort used all-time |
| @@ -451,22 +606,30 @@ | ||
| 451 | 606 | * entries while the column used entries from the tracking window, so a form |
| 452 | 607 | * rendering a dash sorted as though its rate were several hundred percent. |
| 453 | 608 | * Anything needing these numbers must come through here. |
| 454 | 609 | * |
| 610 | + * Both halves apply the same test. Views are not counted for anyone who can | |
| 611 | + * edit the site (Form_Views::should_track()), so their submissions must not be | |
| 612 | + * counted either: testing your own form five times would otherwise add five to | |
| 613 | + * the numerator and nothing to the denominator, and report a rate several times | |
| 614 | + * the real one. The test is applied to capability as it stands now on both | |
| 615 | + * sides, so a site that promotes or demotes people still sees some drift -- | |
| 616 | + * get_editing_submitter_ids() has the detail. The Entries column is unaffected | |
| 617 | + * and stays a true all-time count of every entry received. | |
| 618 | + * | |
| 455 | 619 | * Returns `null` for the rate rather than a number whenever it cannot be |
| 456 | - * measured — tracking off, window never opened, no views yet, or more entries | |
| 457 | - * than views. That last case is not possible in reality (every entry needs a | |
| 458 | - * view first), so it means the view count is incomplete and any percentage | |
| 459 | - * would be invented; the table renders the dash instead. `0.0` is reserved for | |
| 460 | - * a real measurement of zero. | |
| 620 | + * measured: tracking off, window never opened, no views yet, or more entries | |
| 621 | + * than views. That last case means the view count is incomplete, or that a | |
| 622 | + * submitter was promoted after submitting, and any percentage would be invented; | |
| 623 | + * the table renders the dash instead. `0.0` is reserved for a real measurement | |
| 624 | + * of zero. | |
| 461 | 625 | * |
| 462 | - * @param int $form_id Form post ID. | |
| 463 | - * @param string $post_date_gmt Form creation date, GMT. Used to skip a redundant count. | |
| 464 | - * @param int $entries_all_time All-time entry count, when the caller already has it. | |
| 626 | + * @param int $form_id Form post ID. | |
| 465 | 627 | * @return array{views:int,conversion_rate:float|null} |
| 628 | + * @since 2.12.7 -- Signature reduced to $form_id. | |
| 466 | 629 | * @since 2.12.6 |
| 467 | 630 | */ |
| 468 | - private function calculate_form_metrics( $form_id, $post_date_gmt = '', $entries_all_time = null ) { | |
| 631 | + private function calculate_form_metrics( $form_id ) { | |
| 469 | 632 | $none = [ |
| 470 | 633 | 'views' => 0, |
| 471 | 634 | 'conversion_rate' => null, |
| 472 | 635 | ]; |
| @@ -490,35 +653,41 @@ | ||
| 490 | 653 | } |
| 491 | 654 | |
| 492 | 655 | // Compare like with like. The Entries column is all-time, but views only start |
| 493 | 656 | // accruing when tracking opens, so the rate counts entries from that same |
| 494 | - // moment — otherwise a form that existed beforehand divides years of entries by | |
| 495 | - // days of views and reports a rate that is pure noise. | |
| 496 | - $form_created = strtotime( (string) $post_date_gmt ); | |
| 657 | + // moment. Otherwise a form that existed beforehand divides years of entries | |
| 658 | + // by days of views and reports a rate that is pure noise. | |
| 659 | + $where = [ | |
| 660 | + [ | |
| 661 | + [ | |
| 662 | + 'key' => 'created_at', | |
| 663 | + 'compare' => '>=', | |
| 664 | + 'value' => self::window_boundary_sql( $window_start ), | |
| 665 | + ], | |
| 666 | + ], | |
| 667 | + ]; | |
| 497 | 668 | |
| 498 | - if ( null !== $entries_all_time && $form_created && $form_created >= $window_start ) { | |
| 499 | - // The form is younger than the window, so every entry it has is already | |
| 500 | - // inside the window and the caller's all-time count is the same number. | |
| 501 | - // Skips a second COUNT per row on the listing. | |
| 502 | - $entries_since = Helper::get_integer_value( $entries_all_time ); | |
| 503 | - } else { | |
| 504 | - $entries_since = Helper::get_integer_value( | |
| 505 | - Entries::get_total_entries_by_status( | |
| 506 | - 'all', | |
| 507 | - $form_id, | |
| 508 | - [ | |
| 509 | - [ | |
| 510 | - [ | |
| 511 | - 'key' => 'created_at', | |
| 512 | - 'compare' => '>=', | |
| 513 | - 'value' => self::window_boundary_sql( $window_start ), | |
| 514 | - ], | |
| 515 | - ], | |
| 516 | - ] | |
| 517 | - ) | |
| 518 | - ); | |
| 669 | + $editing_users = self::get_editing_submitter_ids( $window_start ); | |
| 670 | + | |
| 671 | + if ( [] !== $editing_users ) { | |
| 672 | + $where[] = [ | |
| 673 | + [ | |
| 674 | + 'key' => 'user_id', | |
| 675 | + 'compare' => 'NOT IN', | |
| 676 | + 'value' => $editing_users, | |
| 677 | + ], | |
| 678 | + ]; | |
| 519 | 679 | } |
| 520 | 680 | |
| 681 | + // Always counted, never taken from the caller's all-time total. That total | |
| 682 | + // includes the entries this exclusion exists to drop, so reusing it for a | |
| 683 | + // form created inside the window -- the newly built form an admin has just | |
| 684 | + // been testing, which is exactly the case that skews -- would hand back the | |
| 685 | + // unfiltered number and quietly undo the exclusion. | |
| 686 | + $entries_since = Helper::get_integer_value( | |
| 687 | + Entries::get_total_entries_by_status( 'all', $form_id, $where ) | |
| 688 | + ); | |
| 689 | + | |
| 521 | 690 | if ( $entries_since > $views ) { |
| 522 | 691 | return [ |
| 523 | 692 | 'views' => $views, |
| 524 | 693 | 'conversion_rate' => null, |
| @@ -545,9 +714,9 @@ | ||
| 545 | 714 | $entries_count = Helper::get_integer_value( Entries::get_total_entries_by_status( 'all', $form_id ) ); |
| 546 | 715 | |
| 547 | 716 | // Views and conversion rate come from the same helper the sort path uses, so the |
| 548 | 717 | // column can never order by a different number than it displays. |
| 549 | - $metrics = $this->calculate_form_metrics( $form_id, $post->post_date_gmt, $entries_count ); | |
| 718 | + $metrics = $this->calculate_form_metrics( $form_id ); | |
| 550 | 719 | $views = $metrics['views']; |
| 551 | 720 | $conversion_rate = $metrics['conversion_rate']; |
| 552 | 721 | |
| 553 | 722 | return [ |
