Bug 1806896 - Move flag_state_activity to core as flag_activity by Xzzz · Pull Request #2657 · mozilla-bteam/bmo

Xzzz · 2026-07-01T18:07:21Z

Summary

Moves flag change tracking infrastructure from the Review extension into Bugzilla core, and renames the table from flag_state_activity to flag_activity.

Adds Bugzilla::FlagActivity as a core ORM class (moved from Bugzilla::Extension::Review::FlagStateActivity)
Adds flag_activity table to Bugzilla::DB::Schema; checksetup.pl renames the existing flag_state_activity table on first run
Bugzilla::Flag now writes all flag changes to flag_activity (create, update, delete) - previously only review, feedback, needinfo and data-review were tracked
Removes the activity-logging hooks from the Review extension (_log_flag_state_activity), which are now redundant; request-count logic (_is_countable_flag) is unchanged
Updates WebService.pm, export_bmo_etl.pl, bug_1022707.pl and remove-non-public-data.pl to reference the new class and table name
Bugzilla::Extension::Review::FlagStateActivity is kept as a thin backward-compat shim inheriting from Bugzilla::FlagActivity

bugs_activity continues to record flagtypes.name entries during the dual-write period.
A follow-up PR will stop the bugs_activity flag writes and switch all consumers (BugMail, GetBugActivity, ActivityStream) to read exclusively from flag_activity once changes has been validated in production.

Test plan

Run checksetup.pl and verify flag_state_activity is renamed to flag_activity
Set, update and clear flags of various types and confirm rows appear in flag_activity for all of them (not just review/feedback/needinfo/data-review)
Confirm bugs_activity still records flagtypes.name entries as before
Verify GET /rest/review/flag_activity still returns correct results

…g.pm

Copilot

Pull request overview

Moves flag change-tracking from the Review extension into Bugzilla core by introducing a new core ORM (Bugzilla::FlagActivity) and core schema table (flag_activity), while updating extension and BMO scripts to use the new class/table name.

Changes:

Add core Bugzilla::FlagActivity ORM and core flag_activity DB schema table.
Update Bugzilla::Flag to write flag create/update/delete activity rows to flag_activity.
Update Review/BMO tooling and provide a compatibility shim for the old extension class name.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
scripts/remove-non-public-data.pl	Switches sanitization logic from `flag_state_activity` to `flag_activity`.
extensions/Review/lib/WebService.pm	Updates REST endpoint implementation to use `Bugzilla::FlagActivity` and renamed helper.
extensions/Review/lib/FlagStateActivity.pm	Converts old extension class into a thin compatibility shim inheriting from core.
extensions/Review/Extension.pm	Removes redundant activity logging hooks and adds migration logic for the renamed table.
extensions/BMO/bin/export_bmo_etl.pl	Updates ETL export path to pull deleted flags from `flag_activity`.
extensions/BMO/bin/bug_1022707.pl	Updates maintenance script to update `flag_activity` instead of `flag_state_activity`.
Bugzilla/FlagActivity.pm	Adds new core ORM for the `flag_activity` table.
Bugzilla/Flag.pm	Writes all flag lifecycle events into `flag_activity`.
Bugzilla/DB/Schema.pm	Adds new core `flag_activity` table definition and indexes.

Comments suppressed due to low confidence (1)

extensions/Review/lib/WebService.pm:273

This helper was renamed to _flag_activity_to_hash, but the parameter is still named $fsa (from the old FlagStateActivity naming). Renaming it avoids confusion and makes the code match the new class/table terminology.

sub _flag_activity_to_hash {
  my ($self, $fsa, $params) = @_;

  my %flag = (
    id            => $self->type('int',    $fsa->id),

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Xzzz · 2026-07-02T13:02:21Z

+use Bugzilla::Error qw(ThrowUserError);
+use Bugzilla::Util qw(trim datetime_from);
+use List::MoreUtils qw(none);


Fixed by modifying line 14 to use Bugzilla::Error qw(ThrowCodeError ThrowUserError);

Or just use use Bugzilla::Error; which will auto import them. This is what is done in most places in the code already.

Line modified to use Bugzilla::Error; (commit "Bug 1806896 - Fix missing ThrowCodeError import in FlagActivity.pm")

Xzzz · 2026-07-02T13:35:02Z

+  # Bug 1806896 - rename flag_state_activity to flag_activity (now in core schema)
+  if ($dbh->bz_table_info('flag_state_activity')
+    && !$dbh->bz_table_info('flag_activity'))
+  {
+    $dbh->do('RENAME TABLE flag_state_activity TO flag_activity');
+  }


Fixed in "Bug 1806896 - Fix flag_state_activity migration and use FK-handling helpers" commit

Xzzz · 2026-07-02T13:38:56Z

+    INDEXES => [
+      flag_activity_flag_id_idx => ['flag_id'],
+      flag_activity_type_id_idx => ['type_id'],
+      flag_activity_bug_id_idx  => ['bug_id'],
+    ],


Fixed in "Bug 1806896 - Add status/flag_when index to flag_activity" commit

…elpers

…ew terminology

dklawren

In both new delete-logging calls (update_flags deletion loop, added after Flag.pm:578, and force_retarget, added after Flag.pm:688), setter_id is Bugzilla->user->id, which is 0 for the anonymous DEFAULT_USER (Bugzilla/User.pm:56). That 0 flows into FlagActivity->create, where _check_param_required('setter_id') runs $value = trim($value) or ThrowCodeError('param_required') — and since trim(0) is falsy, it throws before the DB is even touched (and even if it didn't, setter_id is NOT NULL with an FK to profiles(userid), where no row 0 exists). Because FlagActivity->create runs inside the same transaction as the flag/bug change, the throw aborts and rolls back the entire operation.

Failure scenario: any flag deletion in a no-login context (JobQueue workers, scripts, or any code path reaching update_flags/force_retarget without an authenticated user) now dies. Previously this was limited to countable flags via the Review hook; the move broadens the blast radius to all flag types. The clean fix mirrors the old hook's fallback: setter_id => Bugzilla->user->id || $old_flag->setter_id.

Also confirmed two low-severity latent issues carried over from the old class (now relocated to core), neither a regression: _check_status dereferences $self->id when the create-validator invocant is the class string (crashes with "Can't use string as a HASH reference" only on an invalid status, which the callers never pass), and FlagActivity.pm's type/setter/bug/attachment accessors reference Bugzilla::FlagType/User/Bug/Attachment without use-ing them (works only because callers load them transitively). Adding explicit use lines would harden the now-core module.

dklawren · 2026-07-22T03:19:54Z

 # This Source Code Form is "Incompatible With Secondary Licenses", as
 # defined by the Mozilla Public License, v. 2.0.

+# Bug 1806896 - class moved to core as Bugzilla::FlagActivity


Why not remove this file completely? Is there something else that depends on this being here? I know you mentioned in the PR description to keep it as a shim but I do not see the need for it if nothing in the code is referencing it.

File removed in "Bug 1806896 - Remove unused FlagStateActivity compat shim"

dklawren · 2026-07-22T21:04:01Z

-  }
 }

+


nit: remove extra newline

Fixed in "Bug 1806896 - Remove extra blank line before object_end_of_update"

dklawren · 2026-07-22T21:12:17Z

+    else {
+      $dbh->bz_rename_table('flag_state_activity', 'flag_activity');
+    }
+  }


This should be moved to https://github.com/mozilla-bteam/bmo/blob/master/Bugzilla/Install/DB.pm#L846 as a function called _migrate_flag_state_activity() similar other migration code. This is because it is now considered part of the cor code and DB.pm is place for that.

Claude also mentioned:
Upgraded installs permanently lose the 4 new indexes added to flag_activity if fresh.

bz_setup_database (which runs before extension install_update_db) creates a fresh, fully-indexed core flag_activity. The migration then bz_drop_table('flag_activity') (discarding that indexed table) and bz_rename_table('flag_state_activity','flag_activity') — but the old table had no indexes, and I confirmed in Bugzilla/DB.pm that bz_add_table no-ops on existing tables and there is no generic index reconciliation (only FKs are reconciled via bz_setup_foreign_keys; indexes require explicit bz_add_index). Failure scenario: every existing BMO instance ends up with a flag_activity table that never gets flag_activity_status_when_idx/flag_id/type_id/bug_id indexes, so the ETL WHERE status='X' AND flag_when LIKE ... scan and the WebService match lookups do full table scans forever. Fresh installs get indexes; upgrades silently diverge. Fix: INSERT ... SELECT the old rows into the core table (then drop the old one), or add guarded bz_add_index calls after the rename.

Also something to look at:
Migration silently orphans historical data if flag_activity is already non-empty.

The rename only happens when the freshly-created flag_activity is empty. If the first checksetup run creates the empty core table but aborts before the hook runs, live traffic then writes new rows via the new
Bugzilla::Flag paths; on the next checksetup, new_count > 0 and both tables exist → the block does nothing. There is no data-copy path anywhere, so all historical flag_state_activity rows are stranded and the old table lingers. Fix as above (copy-then-drop is naturally idempotent and safe).

Moved table rename to Bugzilla::Install::DB::_migrate_flag_state_activity() in "Bug 1806896 - Move flag_state_activity migration into Bugzilla::Install::DB"

…exts

…ll::DB

Xzzz · 2026-07-23T21:01:13Z

Fixed the no-login context failure in "Bug 1806896 - Fix setter_id=0 crash on flag deletion in no-login contexts" and the pre-existing bugs you mentioned (not introduced by the relocation) in "Bug 1806896 - Preserve flag_activity indexes during migration" and "Bug 1806896 - Always migrate remaining flag_state_activity rows".

Xzzz added 7 commits July 1, 2026 16:43

Bug 1806896 - Add Bugzilla::FlagActivity core ORM class

4f6ee4c

Bug 1806896 - Add flag_activity table to core schema

69c5c5f

Bug 1806896 - Migrate flag_state_activity table to flag_activity

2179321

Bug 1806896 - Log all flag changes to flag_activity in Flag.pm

cc566e3

Bug 1806896 - Move flag activity logging from Review extension to Fla…

78192e7

…g.pm

Bug 1806896 - Rename flag_state_activity to flag_activity in scripts

e4f6327

Bug 1806896 - Fix FlagStateActivity.pm to satisfy goodperl test

4cbf179

dklawren requested review from Copilot and dklawren July 1, 2026 22:48

Copilot started reviewing on behalf of dklawren July 1, 2026 22:48 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Xzzz added 4 commits July 2, 2026 15:55

Bug 1806896 - Fix flag_state_activity migration and use FK-handling h…

6fadea6

…elpers

Bug 1806896 - Add status/flag_when index to flag_activity

0591975

Bug 1806896 - Rename $fsa to $fa in _flag_activity_to_hash to match n…

9bc7c0b

…ew terminology

Bug 1806896 - Fix missing ThrowCodeError import in FlagActivity.pm

ea1f4b4

dklawren requested changes Jul 22, 2026

View reviewed changes

Xzzz added 6 commits July 23, 2026 20:31

Bug 1806896 - Fix setter_id=0 crash on flag deletion in no-login cont…

4e2a9eb

…exts

Bug 1806896 - Remove extra blank line before object_end_of_update

04fb904

Bug 1806896 - Move flag_state_activity migration into Bugzilla::Insta…

480c9aa

…ll::DB

Bug 1806896 - Remove unused FlagStateActivity compat shim

1dae450

Bug 1806896 - Preserve flag_activity indexes during migration

aecd337

Bug 1806896 - Always migrate remaining flag_state_activity rows

aea9d72

Uh oh!

Conversation

Xzzz commented Jul 1, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dklawren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xzzz commented Jul 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants