From 33902433550faedf2412edbb6b9b7ac8e7e800af Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 19:41:50 +0000 Subject: [PATCH 1/6] fix(merging): backend logic resulting in name collision Allow name merges in the future for merging across chapters, but default to false because renaming activists based on timestamps when merging within a chapter from the UI may be a confusing UX. --- server/src/main.go | 3 +- server/src/model/activist.go | 49 ++++++------ server/src/model/activist_test.go | 121 +++++++++++++++++++++--------- 3 files changed, 115 insertions(+), 58 deletions(-) diff --git a/server/src/main.go b/server/src/main.go index fd7949fc..87580d0e 100644 --- a/server/src/main.go +++ b/server/src/main.go @@ -1110,6 +1110,7 @@ func (c MainController) ActivistMergeHandler(w http.ResponseWriter, r *http.Requ var activistMergeData struct { CurrentActivistID int `json:"current_activist_id"` TargetActivistName string `json:"target_activist_name"` + MergeName bool `json:"merge_name"` } err := json.NewDecoder(r.Body).Decode(&activistMergeData) if err != nil { @@ -1125,7 +1126,7 @@ func (c MainController) ActivistMergeHandler(w http.ResponseWriter, r *http.Requ return } - err = model.MergeActivist(c.db, activistMergeData.CurrentActivistID, mergedActivist.ID) + err = model.MergeActivist(c.db, activistMergeData.CurrentActivistID, mergedActivist.ID, activistMergeData.MergeName) if err != nil { sendErrorMessage(w, err) return diff --git a/server/src/model/activist.go b/server/src/model/activist.go index d1b3c79a..42bef325 100644 --- a/server/src/model/activist.go +++ b/server/src/model/activist.go @@ -1359,7 +1359,7 @@ func HideActivist(db *sqlx.DB, activistID int) error { // Merge activistID into targetActivistID. // - The original activist is hidden // - All of the original activist's event attendance is updated to be the target activist. -func MergeActivist(db *sqlx.DB, originalActivistID, targetActivistID int) error { +func MergeActivist(db *sqlx.DB, originalActivistID, targetActivistID int, mergeName bool) error { if originalActivistID == 0 { return errors.New("originalActivistID cannot be 0") } @@ -1375,6 +1375,23 @@ func MergeActivist(db *sqlx.DB, originalActivistID, targetActivistID int) error return errors.Wrap(err, "could not create transaction") } + // Read both activists before hideActivistQuery rewrites the original's name to " ". Otherwise, when + // mergeName is true, the field merge would see the synthesized hidden name and try to write it onto the target, + // colliding with the original row that still owns it under the (name, chapter_id) unique key. + // + // Updates are rare so don't bother locking these reads with FOR UPDATE. + query := selectActivistExtraBaseQuery + " WHERE a.id = ?" + originalActivist := new(ActivistExtra) + if err := tx.Get(originalActivist, query, originalActivistID); err != nil { + tx.Rollback() + return fmt.Errorf("failed to get original activist with id %d: %w", originalActivistID, err) + } + targetActivist := new(ActivistExtra) + if err := tx.Get(targetActivist, query, targetActivistID); err != nil { + tx.Rollback() + return fmt.Errorf("failed to get target activist with id %d: %w", targetActivistID, err) + } + _, err = tx.Exec(hideActivistQuery, originalActivistID) if err != nil { tx.Rollback() @@ -1395,7 +1412,7 @@ func MergeActivist(db *sqlx.DB, originalActivistID, targetActivistID int) error return err } - _, err = updateMergedActivistDataDetails(tx, originalActivistID, targetActivistID) + _, err = updateMergedActivistDataDetails(tx, originalActivist, targetActivist, mergeName) if err != nil { tx.Rollback() return err @@ -1500,7 +1517,7 @@ func insertMergedActivistAttendance(tx *sqlx.Tx, originalActivistID int, targetA originalActivistID, targetActivistID) } -func getMergeActivistWinner(original ActivistExtra, target ActivistExtra) ActivistExtra { +func getMergeActivistWinner(original ActivistExtra, target ActivistExtra, mergeName bool) ActivistExtra { levels := map[string]int{ "Supporter": 0, "Non-Local": 1, @@ -1521,7 +1538,9 @@ func getMergeActivistWinner(original ActivistExtra, target ActivistExtra) Activi target.Email, target.EmailUpdated = stringMergeWithTimestamps(original.Email, original.EmailUpdated, target.Email, target.EmailUpdated) target.Phone, target.PhoneUpdated = stringMergeWithTimestamps(original.Phone, original.PhoneUpdated, target.Phone, target.PhoneUpdated) - target.Name, target.NameUpdated = stringMergeWithTimestamps(original.Name, original.NameUpdated, target.Name, target.NameUpdated) + if mergeName { + target.Name, target.NameUpdated = stringMergeWithTimestamps(original.Name, original.NameUpdated, target.Name, target.NameUpdated) + } target.PreferredName = stringMerge(original.PreferredName, target.PreferredName) target.Pronouns = stringMerge(original.Pronouns, target.Pronouns) target.Language = stringMerge(original.Language, target.Language) @@ -1646,31 +1665,17 @@ func mergeAddress(originalAddr ActivistAddress, originalCoords Coords, originalU return addr, newerCoords, newerUpdated } -func updateMergedActivistDataDetails(tx *sqlx.Tx, originalActivistID int, targetActivistID int) (*ActivistExtra, error) { +func updateMergedActivistDataDetails(tx *sqlx.Tx, originalActivist, targetActivist *ActivistExtra, mergeName bool) (*ActivistExtra, error) { // Merge details of original activist into target activist // Favor booleans that are set to TRUE, and pull in missing data from original activist to target; when both // activists have data for the same field, we should use the target activist's data. - query := selectActivistExtraBaseQuery + " WHERE a.id = ?" - - var originalActivist = new(ActivistExtra) - err := tx.Get(originalActivist, query, originalActivistID) - if err != nil { - return nil, errors.Wrapf(err, "failed to get original activist with id %d", originalActivistID) - } - - var targetActivist = new(ActivistExtra) - err = tx.Get(targetActivist, query, targetActivistID) - if err != nil { - return nil, errors.Wrapf(err, "failed to get target activist with id %d", targetActivistID) - } - - mergedActivist := getMergeActivistWinner(*originalActivist, *targetActivist) + mergedActivist := getMergeActivistWinner(*originalActivist, *targetActivist, mergeName) - _, err = tx.NamedExec(updateActivistWithTimestampsQuery, mergedActivist) + _, err := tx.NamedExec(updateActivistWithTimestampsQuery, mergedActivist) if err != nil { - return nil, errors.Wrapf(err, "failed to update activist with id %d", targetActivistID) + return nil, errors.Wrapf(err, "failed to update activist with id %d", targetActivist.ID) } return &mergedActivist, nil diff --git a/server/src/model/activist_test.go b/server/src/model/activist_test.go index 27234abb..375e07ff 100644 --- a/server/src/model/activist_test.go +++ b/server/src/model/activist_test.go @@ -702,10 +702,11 @@ func TestPatchActivist_ValidatesAssignedTo(t *testing.T) { } func TestMergeActivist(t *testing.T) { + db := testdb.NewDB() + defer db.Close() + t.Run("ContactInfo", func(t *testing.T) { t.Run("MergesNewerValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() a1 := NewActivistBuilder(). WithEmail("berkeley@example.org"). @@ -723,7 +724,7 @@ func TestMergeActivist(t *testing.T) { a2.PhoneUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, "berkeley@example.org", a2.Email) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.EmailUpdated) @@ -732,9 +733,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("DoesNotMergeNewerButEmptyValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder(). WithEmail(""). WithPhone(""). @@ -751,7 +749,7 @@ func TestMergeActivist(t *testing.T) { a2.PhoneUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, "old@example.org", a2.Email) assert.Equal(t, mustParseTime(t, "2025-01-01"), a2.EmailUpdated) @@ -760,9 +758,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("DoesNotMergeOlderValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder(). WithEmail("old@example.org"). WithPhone("510-555-0000"). @@ -779,7 +774,7 @@ func TestMergeActivist(t *testing.T) { a2.PhoneUpdated = mustParseTime(t, "2025-01-02") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, "berkeley@example.org", a2.Email) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.EmailUpdated) @@ -788,11 +783,82 @@ func TestMergeActivist(t *testing.T) { }) }) + t.Run("Name", func(t *testing.T) { + t.Run("MergeNameTrueMergesNewerNameOntoTarget", func(t *testing.T) { + a1 := NewActivistBuilder().WithName("Alex Barnes").Build() + a1.NameUpdated = mustParseTime(t, "2025-01-02") + MustInsertActivistWithTimestamps(t, db, a1) + + a2 := NewActivistBuilder().WithName("Alex Davis").Build() + a2.NameUpdated = mustParseTime(t, "2025-01-01") + MustInsertActivistWithTimestamps(t, db, a2) + + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, true)) + + merged := MustGetActivist(t, db, a2.ID) + assert.Equal(t, "Alex Barnes", merged.Name) + assert.Equal(t, mustParseTime(t, "2025-01-02"), merged.NameUpdated) + + var hidden struct { + Name string `db:"name"` + Hidden bool `db:"hidden"` + } + require.NoError(t, db.Get(&hidden, "SELECT name, hidden FROM activists WHERE id = ?", a1.ID)) + assert.True(t, hidden.Hidden) + assert.Equal(t, "Alex Barnes "+strconv.Itoa(a1.ID), hidden.Name) + }) + + t.Run("MergeNameTrueSourceIsOlderPreservesTargetName", func(t *testing.T) { + a1 := NewActivistBuilder().WithName("Alex Older").Build() + a1.NameUpdated = mustParseTime(t, "2025-01-01") + MustInsertActivistWithTimestamps(t, db, a1) + + a2 := NewActivistBuilder().WithName("Alex Newer").Build() + a2.NameUpdated = mustParseTime(t, "2025-01-02") + MustInsertActivistWithTimestamps(t, db, a2) + + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, true)) + + merged := MustGetActivist(t, db, a2.ID) + assert.Equal(t, "Alex Newer", merged.Name) + assert.Equal(t, mustParseTime(t, "2025-01-02"), merged.NameUpdated) + + var hidden struct { + Name string `db:"name"` + Hidden bool `db:"hidden"` + } + require.NoError(t, db.Get(&hidden, "SELECT name, hidden FROM activists WHERE id = ?", a1.ID)) + assert.True(t, hidden.Hidden) + assert.Equal(t, "Alex Older "+strconv.Itoa(a1.ID), hidden.Name) + }) + + t.Run("MergeNameFalsePreservesTargetName", func(t *testing.T) { + a1 := NewActivistBuilder().WithName("Alex Duplicate").Build() + a1.NameUpdated = mustParseTime(t, "2025-01-02") + MustInsertActivistWithTimestamps(t, db, a1) + + a2 := NewActivistBuilder().WithName("Alex Target").Build() + a2.NameUpdated = mustParseTime(t, "2025-01-01") + MustInsertActivistWithTimestamps(t, db, a2) + + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) + + merged := MustGetActivist(t, db, a2.ID) + assert.Equal(t, "Alex Target", merged.Name) + assert.Equal(t, mustParseTime(t, "2025-01-01"), merged.NameUpdated) + + var hidden struct { + Name string `db:"name"` + Hidden bool `db:"hidden"` + } + require.NoError(t, db.Get(&hidden, "SELECT name, hidden FROM activists WHERE id = ?", a1.ID)) + assert.True(t, hidden.Hidden) + assert.Equal(t, "Alex Duplicate "+strconv.Itoa(a1.ID), hidden.Name) + }) + }) + t.Run("Address", func(t *testing.T) { t.Run("MergesNewerValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder(). WithAddress("100 Berkeley Way", "Berkeley", "CA"). WithCoords(1, 2). @@ -807,7 +873,7 @@ func TestMergeActivist(t *testing.T) { a2.AddressUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.AddressUpdated) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.LocationUpdated) @@ -816,9 +882,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("DoesNotMergeNewerButEmptyValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder(). WithAddress("", "", ""). WithCoords(0, 0). @@ -833,7 +896,7 @@ func TestMergeActivist(t *testing.T) { a2.AddressUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, mustParseTime(t, "2025-01-01"), a2.AddressUpdated) assert.Equal(t, "200 Berkeley Way", a2.StreetAddress) @@ -841,9 +904,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("DoesNotMergeOlderValues", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder(). WithAddress("100 Berkeley Way", "Berkeley", "CA"). WithCoords(1, 2). @@ -858,7 +918,7 @@ func TestMergeActivist(t *testing.T) { a2.AddressUpdated = mustParseTime(t, "2025-01-02") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.AddressUpdated) assert.Equal(t, "200 Berkeley Way", a2.StreetAddress) @@ -866,9 +926,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("MergesAddressWhenCityMatches", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder().WithAddress("100 Berkeley Way", "Berkeley", "CA").Build() a1.AddressUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a1) @@ -877,16 +934,13 @@ func TestMergeActivist(t *testing.T) { a2.AddressUpdated = mustParseTime(t, "2025-01-02") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.AddressUpdated) assert.Equal(t, "100 Berkeley Way", a2.StreetAddress) }) t.Run("DoesNotMergeAddressWhenCityNotMatched", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - a1 := NewActivistBuilder().WithAddress("100 Berkeley Way", "Berkeley", "CA").Build() a1.AddressUpdated = mustParseTime(t, "2025-01-01") MustInsertActivistWithTimestamps(t, db, a1) @@ -895,7 +949,7 @@ func TestMergeActivist(t *testing.T) { a2.AddressUpdated = mustParseTime(t, "2025-01-02") MustInsertActivistWithTimestamps(t, db, a2) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) a2 = MustGetActivist(t, db, a2.ID) assert.Equal(t, mustParseTime(t, "2025-01-02"), a2.AddressUpdated) assert.Equal(t, "", a2.StreetAddress) @@ -903,9 +957,6 @@ func TestMergeActivist(t *testing.T) { }) t.Run("MergesEvents", func(t *testing.T) { - db := testdb.NewDB() - defer db.Close() - // Test that deleting activists works a1, err := GetOrCreateActivist(db, "Test Activist", SFBayChapterIdDevTest) require.NoError(t, err) @@ -941,7 +992,7 @@ func TestMergeActivist(t *testing.T) { }} mustInsertAllEvents(t, db, insertEvents) - require.NoError(t, MergeActivist(db, a1.ID, a2.ID)) + require.NoError(t, MergeActivist(db, a1.ID, a2.ID, false)) e1, err := GetEvent(db, GetEventOptions{EventID: 1}) require.NoError(t, err) From cc17fd0ea7599e8c46301aed245e98dee4b5691d Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 21:37:43 +0000 Subject: [PATCH 2/6] feat: allow non-sfbay chapters to hide activists --- server/src/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main.go b/server/src/main.go index 87580d0e..39c51e8e 100644 --- a/server/src/main.go +++ b/server/src/main.go @@ -318,7 +318,7 @@ func router() (*mux.Router, *sqlx.DB) { router.Handle("/activist/list", alice.New(main.apiOrganizerAccessAuthMiddleware).ThenFunc(main.ActivistListHandler)) router.Handle("/activist/list_basic", alice.New(main.apiAttendanceAuthMiddleware).ThenFunc(main.ActivistListBasicHandler)) router.Handle("/activist/save", alice.New(main.apiOrganizerAccessAuthMiddleware).ThenFunc(main.ActivistSaveHandler)) - router.Handle("/activist/hide", alice.New(main.apiSFBayOrganizerAuthMiddleware).ThenFunc(main.ActivistHideHandler)) + router.Handle("/activist/hide", alice.New(main.apiOrganizerAccessAuthMiddleware).ThenFunc(main.ActivistHideHandler)) router.Handle("/activist/merge", alice.New(main.apiOrganizerAccessAuthMiddleware).ThenFunc(main.ActivistMergeHandler)) router.Handle("/working_group/save", alice.New(main.apiSFBayOrganizerAuthMiddleware).ThenFunc(main.WorkingGroupSaveHandler)) router.Handle("/working_group/list", alice.New(main.apiAttendanceAuthMiddleware).ThenFunc(main.WorkingGroupListHandler)) From 239e275be05df7a5d6d9cdc0e107128aaad3594a Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 21:59:15 +0000 Subject: [PATCH 3/6] fix(backend): verify user chapter for various endpoints --- server/src/main.go | 20 +++++++++++++++---- server/src/model/activist.go | 32 ++++++++++++++++--------------- server/src/model/activist_test.go | 6 ++++-- server/src/model/adb_auth.go | 15 +++++++++++++++ 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/server/src/main.go b/server/src/main.go index 39c51e8e..eb714c16 100644 --- a/server/src/main.go +++ b/server/src/main.go @@ -1061,7 +1061,7 @@ func (c MainController) ActivistSaveHandler(w http.ResponseWriter, r *http.Reque activistExtra.ChapterID = user.ChapterID activistID, err = model.CreateActivist(c.db, activistExtra) } else { - activistID, err = model.UserUpdateActivist(c.db, activistExtra, user.Email, c.userRepo) + activistID, err = model.UserUpdateActivist(c.db, activistExtra, user, c.userRepo) } if err != nil { sendErrorMessage(w, err) @@ -1083,6 +1083,8 @@ func (c MainController) ActivistSaveHandler(w http.ResponseWriter, r *http.Reque } func (c MainController) ActivistHideHandler(w http.ResponseWriter, r *http.Request) { + user, _ := c.getAuthedADBUser(r) + var activistID struct { ID int `json:"id"` } @@ -1092,7 +1094,7 @@ func (c MainController) ActivistHideHandler(w http.ResponseWriter, r *http.Reque return } - err = model.HideActivist(c.db, activistID.ID) + err = model.HideActivist(c.db, user, activistID.ID) if err != nil { sendErrorMessage(w, err) return @@ -1105,7 +1107,7 @@ func (c MainController) ActivistHideHandler(w http.ResponseWriter, r *http.Reque } func (c MainController) ActivistMergeHandler(w http.ResponseWriter, r *http.Request) { - chapter := c.getAuthedADBChapter(r) + user, _ := c.getAuthedADBUser(r) var activistMergeData struct { CurrentActivistID int `json:"current_activist_id"` @@ -1118,9 +1120,19 @@ func (c MainController) ActivistMergeHandler(w http.ResponseWriter, r *http.Requ return } + orig, err := model.GetActivistsExtra(c.db, model.GetActivistOptions{ID: activistMergeData.CurrentActivistID}) + if err != nil || len(orig) == 0 { + sendErrorMessage(w, fmt.Errorf("activist with id %d not found", activistMergeData.CurrentActivistID)) + return + } + if err := model.CheckChapterAccess(user, orig[0].ChapterID); err != nil { + sendErrorMessage(w, err) + return + } + // First, we need to get the activist ID for the target // activist. - mergedActivist, err := model.GetActivist(c.db, activistMergeData.TargetActivistName, chapter) + mergedActivist, err := model.GetActivist(c.db, activistMergeData.TargetActivistName, user.ChapterID) if err != nil { sendErrorMessage(w, errors.Wrapf(err, "Could not fetch data for: %s", activistMergeData.TargetActivistName)) return diff --git a/server/src/model/activist.go b/server/src/model/activist.go index 42bef325..1c969679 100644 --- a/server/src/model/activist.go +++ b/server/src/model/activist.go @@ -1211,7 +1211,7 @@ func geocodeIfAddressChanged(orig ActivistExtra, updated *ActivistExtra) { } // UserUpdateActivist updates user-editable activist fields as requested by an ADB user. -func UserUpdateActivist(db *sqlx.DB, activist ActivistExtra, userEmail string, userRepo UserRepository) (int, error) { +func UserUpdateActivist(db *sqlx.DB, activist ActivistExtra, authedUser ADBUser, userRepo UserRepository) (int, error) { if activist.ID == 0 { return 0, errors.New("activist ID cannot be 0") } @@ -1225,6 +1225,10 @@ func UserUpdateActivist(db *sqlx.DB, activist ActivistExtra, userEmail string, u } origActivist := orig[0] + if err := CheckChapterAccess(authedUser, origActivist.ChapterID); err != nil { + return 0, err + } + if err := validateActivistUpdate(origActivist, activist, userRepo); err != nil { return 0, err } @@ -1240,7 +1244,7 @@ func UserUpdateActivist(db *sqlx.DB, activist ActivistExtra, userEmail string, u log.Printf("Updated data for activist %v", activist.Name) - addActivistHistory(db, userEmail, activist) + addActivistHistory(db, authedUser.Email, activist) return activist.ID, nil } @@ -1301,13 +1305,8 @@ func PatchActivist(db *sqlx.DB, repo ActivistRepository, userRepo UserRepository orig := origActivists[0] // Non-admin users can only update activists in their own chapter. - if !UserHasRole(shared.RoleAdmin, authedUser) { - if orig.ChapterID == 0 || authedUser.ChapterID == 0 { - return ValidationErrorf("activist chapter check failed") - } - if orig.ChapterID != authedUser.ChapterID { - return ValidationErrorf("activist does not belong to your chapter") - } + if err := CheckChapterAccess(authedUser, orig.ChapterID); err != nil { + return err } merged, err := patch.ApplyTo(orig) @@ -1336,17 +1335,20 @@ func PatchActivist(db *sqlx.DB, repo ActivistRepository, userRepo UserRepository return nil } -func HideActivist(db *sqlx.DB, activistID int) error { +func HideActivist(db *sqlx.DB, authedUser ADBUser, activistID int) error { if activistID == 0 { return errors.New("HideActivist: activistID cannot be 0") } - var activistCount int - err := db.Get(&activistCount, `SELECT count(*) FROM activists WHERE id = ?`, activistID) + var existing struct { + ChapterID int `db:"chapter_id"` + } + err := db.Get(&existing, `SELECT chapter_id FROM activists WHERE id = ?`, activistID) if err != nil { - return errors.Wrap(err, "failed to get activist count") + return errors.Wrapf(err, "activist with id %d does not exist", activistID) } - if activistCount == 0 { - return errors.Errorf("Activist with id %d does not exist", activistID) + + if err := CheckChapterAccess(authedUser, existing.ChapterID); err != nil { + return err } _, err = db.Exec(hideActivistQuery, activistID) diff --git a/server/src/model/activist_test.go b/server/src/model/activist_test.go index 375e07ff..159fe402 100644 --- a/server/src/model/activist_test.go +++ b/server/src/model/activist_test.go @@ -412,7 +412,8 @@ func TestUpdateActivist(t *testing.T) { activist.Coords = Coords{1, 2} u := MakeUserRepoStub(t, nil) - updatedID, err := UserUpdateActivist(db, *activist, DevTestUserEmail, u) + authedUser := ADBUser{ID: DevTestUserId, Email: DevTestUserEmail, Roles: []string{shared.RoleAdmin}} + updatedID, err := UserUpdateActivist(db, *activist, authedUser, u) require.NoError(t, err) require.Equal(t, id, updatedID) @@ -450,7 +451,8 @@ func TestHideActivist(t *testing.T) { }) require.NoError(t, err) - require.NoError(t, HideActivist(db, a1.ID)) + authedAdmin := ADBUser{ID: DevTestUserId, Email: DevTestUserEmail, Roles: []string{shared.RoleAdmin}} + require.NoError(t, HideActivist(db, authedAdmin, a1.ID)) // Hidden activists should not show up in the autocompleted names names := GetAutocompleteNames(db, SFBayChapterIdDevTest) diff --git a/server/src/model/adb_auth.go b/server/src/model/adb_auth.go index 31f9d850..0550e666 100644 --- a/server/src/model/adb_auth.go +++ b/server/src/model/adb_auth.go @@ -116,6 +116,21 @@ func UserHasRole(role string, user ADBUser) bool { return false } +// CheckChapterAccess returns nil if the user is an admin or if chapterID matches +// the user's chapter. Returns a ValidationError otherwise. +func CheckChapterAccess(user ADBUser, chapterID int) error { + if UserHasRole(shared.RoleAdmin, user) { + return nil + } + if chapterID == 0 || user.ChapterID == 0 { + return ValidationErrorf("activist chapter check failed") + } + if chapterID != user.ChapterID { + return ValidationErrorf("activist does not belong to your chapter") + } + return nil +} + // Interface for querying and updating users. This avoids a dependency on the persistence package which could create a // cyclical package reference. type UserRepository interface { From 77de95abe5b5a4fbf052e6a783bdf5ed99b979b4 Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 22:02:13 +0000 Subject: [PATCH 4/6] fix: disallow merging across chapters --- server/src/model/activist.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/server/src/model/activist.go b/server/src/model/activist.go index 1c969679..534998c6 100644 --- a/server/src/model/activist.go +++ b/server/src/model/activist.go @@ -1394,6 +1394,14 @@ func MergeActivist(db *sqlx.DB, originalActivistID, targetActivistID int, mergeN return fmt.Errorf("failed to get target activist with id %d: %w", targetActivistID, err) } + // Don't allow merging across chapters. Among potential other issues, this + // would merge attendance data from one chapter into an activist in + // another chapter which is not supported. + if originalActivist.ChapterID != targetActivist.ChapterID { + tx.Rollback() + return ValidationErrorf("cannot merge activists from different chapters") + } + _, err = tx.Exec(hideActivistQuery, originalActivistID) if err != nil { tx.Rollback() From b7313760eb003d0745c694929d33b10921ae4c6d Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 19:44:14 +0000 Subject: [PATCH 5/6] feat(activists): merge and delete in frontend-v2 --- .../activists/[id]/activist-detail.tsx | 41 ++++- .../activists/[id]/hide-activist-dialog.tsx | 84 +++++++++ .../activists/[id]/merge-activist-dialog.tsx | 171 ++++++++++++++++++ frontend-v2/src/lib/api.ts | 45 +++++ 4 files changed, 339 insertions(+), 2 deletions(-) create mode 100644 frontend-v2/src/app/(authed)/activists/[id]/hide-activist-dialog.tsx create mode 100644 frontend-v2/src/app/(authed)/activists/[id]/merge-activist-dialog.tsx diff --git a/frontend-v2/src/app/(authed)/activists/[id]/activist-detail.tsx b/frontend-v2/src/app/(authed)/activists/[id]/activist-detail.tsx index e0e4dae2..e15bed9c 100644 --- a/frontend-v2/src/app/(authed)/activists/[id]/activist-detail.tsx +++ b/frontend-v2/src/app/(authed)/activists/[id]/activist-detail.tsx @@ -3,7 +3,7 @@ import { useCallback, useEffect, useMemo, useState } from 'react' import { useQuery } from '@tanstack/react-query' import Link from 'next/link' -import { ArrowLeft, Pencil } from 'lucide-react' +import { ArrowLeft, EyeOff, GitMerge, Pencil } from 'lucide-react' import { API_PATH, apiClient, @@ -22,6 +22,8 @@ import { FieldDescriptionPopover } from '../field-description-popover' import { formatValue } from '../format-value' import { LinkedValue } from '../linked-value' import { ActivistSectionForm } from './section-form' +import { HideActivistDialog } from './hide-activist-dialog' +import { MergeActivistDialog } from './merge-activist-dialog' const NOTES_SECTION_KEY = '__notes__' type SectionKey = ColumnCategory | typeof NOTES_SECTION_KEY @@ -67,6 +69,8 @@ export function ActivistDetail({ activistId }: { activistId: number }) { const { data: activist, isError, isLoading } = useActivist(activistId) const [editingSection, setEditingSection] = useState(null) const [isFormDirty, setIsFormDirty] = useState(false) + const [isHideDialogOpen, setIsHideDialogOpen] = useState(false) + const [isMergeDialogOpen, setIsMergeDialogOpen] = useState(false) const confirmDiscard = useCallback(() => { if (!isFormDirty) return true @@ -145,7 +149,7 @@ export function ActivistDetail({ activistId }: { activistId: number }) { -
+

{displayName.text}

+
+ + +
+ + +
{SECTION_ORDER.map((category) => { const fields = groupedFields.get(category) ?? [] diff --git a/frontend-v2/src/app/(authed)/activists/[id]/hide-activist-dialog.tsx b/frontend-v2/src/app/(authed)/activists/[id]/hide-activist-dialog.tsx new file mode 100644 index 00000000..6f9db9de --- /dev/null +++ b/frontend-v2/src/app/(authed)/activists/[id]/hide-activist-dialog.tsx @@ -0,0 +1,84 @@ +'use client' + +import { useRouter } from 'next/navigation' +import { useMutation, useQueryClient } from '@tanstack/react-query' +import toast from 'react-hot-toast' +import { API_PATH, apiClient } from '@/lib/api' +import { Button } from '@/components/ui/button' +import { + Dialog, + DialogContent, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog' + +type Props = { + open: boolean + onOpenChange: (open: boolean) => void + activistId: number + activistName: string +} + +export function HideActivistDialog({ + open, + onOpenChange, + activistId, + activistName, +}: Props) { + const router = useRouter() + const queryClient = useQueryClient() + + const mutation = useMutation({ + mutationFn: () => apiClient.hideActivist(activistId), + onSuccess: () => { + toast.success(`${activistName} was hidden`) + queryClient.invalidateQueries({ queryKey: [API_PATH.ACTIVISTS_SEARCH] }) + queryClient.invalidateQueries({ + queryKey: [API_PATH.ACTIVIST_LIST_BASIC], + }) + onOpenChange(false) + router.push('/activists') + }, + onError: (err: Error) => { + toast.error(err.message || 'Failed to hide activist') + }, + }) + + const handleOpenChange = (next: boolean) => { + if (mutation.isPending) return + onOpenChange(next) + } + + return ( + + + + Hide activist + +

+ WARNING: Hiding this activist will make them inaccessible unless they + are unhidden by Tech. Are you sure you want to hide this activist? +

+ + + + + + + ) +} diff --git a/frontend-v2/src/app/(authed)/activists/[id]/merge-activist-dialog.tsx b/frontend-v2/src/app/(authed)/activists/[id]/merge-activist-dialog.tsx new file mode 100644 index 00000000..5017544b --- /dev/null +++ b/frontend-v2/src/app/(authed)/activists/[id]/merge-activist-dialog.tsx @@ -0,0 +1,171 @@ +'use client' + +import { useState } from 'react' +import { useRouter } from 'next/navigation' +import { useMutation, useQueryClient } from '@tanstack/react-query' +import toast from 'react-hot-toast' +import { API_PATH, apiClient } from '@/lib/api' +import { useAuthedPageContext } from '@/hooks/useAuthedPageContext' +import { useActivistRegistry } from '../../events/useActivistRegistry' +import { SuggestionInput } from '../../events/suggestion-input' +import { Button } from '@/components/ui/button' +import { Label } from '@/components/ui/label' +import { + Dialog, + DialogContent, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog' + +type Props = { + open: boolean + onOpenChange: (open: boolean) => void + activistId: number + activistName: string +} + +export function MergeActivistDialog({ + open, + onOpenChange, + activistId, + activistName, +}: Props) { + const router = useRouter() + const queryClient = useQueryClient() + const { user } = useAuthedPageContext() + const { registry } = useActivistRegistry(user.ChapterID) + + // Raw text in the input as the user types. + const [inputValue, setInputValue] = useState('') + // Name of an activist explicitly selected from the suggestion list. + const [selectedValue, setSelectedValue] = useState('') + + const mutation = useMutation({ + mutationFn: (targetName: string) => + apiClient.mergeActivist(activistId, targetName), + onSuccess: (_data, targetName) => { + toast.success(`${activistName} was merged into ${targetName}`) + queryClient.invalidateQueries({ queryKey: [API_PATH.ACTIVISTS_SEARCH] }) + queryClient.invalidateQueries({ + queryKey: [API_PATH.ACTIVIST_LIST_BASIC], + }) + onOpenChange(false) + router.push('/activists') + }, + onError: (err: Error) => { + toast.error(err.message || 'Failed to merge activist') + }, + }) + + const handleOpenChange = (next: boolean) => { + if (mutation.isPending) return + if (!next) { + setInputValue('') + setSelectedValue('') + } + onOpenChange(next) + } + + const getSuggestions = (input: string) => + registry.getSuggestions(input).filter((name) => name !== activistName) + + const isTargetValid = + selectedValue.trim().length > 0 && + // Cannot merge activist with self. (Names are unique within chapter, and + // backend endpoint will not allow merging across chapters.) + selectedValue !== activistName && + registry.getActivist(selectedValue) !== null + + const handleSubmit = () => { + if (!isTargetValid) { + toast.error('Choose an activist to merge into') + return + } + mutation.mutate(selectedValue) + } + + return ( + + + + Merge activist + +
+

+ Merging activists is used to combine redundant activist entries. +

+

Merging this activist does the following:

+
    +
  • + All of {activistName}'s event attendance will be moved to the + target activist. +
    • +
  • + The target's name will be kept — {activistName}'s + name will not replace it. +
    • +
  • + {activistName}'s email, phone, and address / location will + replace the corresponding fields in the target{' '} + if updated more recently. +
    • +
  • + {activistName}'s pronouns, language, notes, and most other + fields will each replace the corresponding field in the target{' '} + only if the target's value is blank. +
    • +
  • + Yes / no flags (e.g. MPI, hiatus, voting agreement) will be set to + “yes” if either activist has them set. +
    • +
  • + The target's activist level will be set to the{' '} + higher of the two activists' levels. +
    • +
  • {activistName} will be hidden.
    • +
+

+ Merge {activistName} into another activist: +

+
+
+ + { + setInputValue(v) + setSelectedValue('') + }} + getSuggestions={getSuggestions} + onCommit={(meta) => { + if (meta.fromSuggestion) { + setSelectedValue(meta.value) + } + }} + placeholder="Start typing a name..." + /> +
+ + + + + + + ) +} diff --git a/frontend-v2/src/lib/api.ts b/frontend-v2/src/lib/api.ts index 1a03a60a..e9ee3ab7 100644 --- a/frontend-v2/src/lib/api.ts +++ b/frontend-v2/src/lib/api.ts @@ -17,6 +17,8 @@ export const API_PATH = { ACTIVIST_LIST_BASIC: 'activist/list_basic', ACTIVISTS_SEARCH: 'api/activists', ACTIVIST_GET: 'api/activists', + ACTIVIST_HIDE: 'activist/hide', + ACTIVIST_MERGE: 'activist/merge', USER_ME: 'user/me', CSRF_TOKEN: 'api/csrf-token', CHAPTER_LIST: 'chapter/list', @@ -283,6 +285,10 @@ const EventDeleteResp = z.object({ status: z.literal('success'), }) +const SuccessResp = z.object({ + status: z.literal('success'), +}) + const ApiErrorResp = z.object({ status: z.literal('error'), message: z.string(), @@ -444,6 +450,45 @@ export class ApiClient { } } + hideActivist = async (activistId: number, signal?: AbortSignal) => { + try { + // TODO: pass X-CSRF-Token header once the backend requires it for this endpoint. + const resp = await this.client + .post(API_PATH.ACTIVIST_HIDE, { + json: { id: activistId }, + signal, + }) + .json() + this.throwIfApiError(resp) + return SuccessResp.parse(resp) + } catch (err) { + return this.handleKyError(err) + } + } + + mergeActivist = async ( + currentActivistId: number, + targetActivistName: string, + signal?: AbortSignal, + ) => { + try { + // TODO: pass X-CSRF-Token header once the backend requires it for this endpoint. + const resp = await this.client + .post(API_PATH.ACTIVIST_MERGE, { + json: { + current_activist_id: currentActivistId, + target_activist_name: targetActivistName, + }, + signal, + }) + .json() + this.throwIfApiError(resp) + return SuccessResp.parse(resp) + } catch (err) { + return this.handleKyError(err) + } + } + getChapterList = async (signal?: AbortSignal) => { try { const resp = await this.client From be064bb974d4df753e8235456cb3b04e8a857a74 Mon Sep 17 00:00:00 2001 From: Alexander Taylor Date: Sun, 3 May 2026 22:27:51 +0000 Subject: [PATCH 6/6] refactor(frontend): consolidate response types --- frontend-v2/src/lib/api.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/frontend-v2/src/lib/api.ts b/frontend-v2/src/lib/api.ts index e9ee3ab7..75e57df1 100644 --- a/frontend-v2/src/lib/api.ts +++ b/frontend-v2/src/lib/api.ts @@ -281,10 +281,6 @@ export interface EventListParams { event_type: EventType } -const EventDeleteResp = z.object({ - status: z.literal('success'), -}) - const SuccessResp = z.object({ status: z.literal('success'), }) @@ -636,7 +632,7 @@ export class ApiClient { }) .json() this.throwIfApiError(resp) - return EventDeleteResp.parse(resp) + return SuccessResp.parse(resp) } catch (err) { return this.handleKyError(err) }