target / goalert

Open source on-call scheduling, automated escalations, and notifications so you never miss a critical alert
https://goalert.me
Apache License 2.0
2.22k stars 240 forks source link

rotation advancing on user remove #3771

Open Forfold opened 6 months ago

Forfold commented 6 months ago

Describe the Bug: When removing a user from a rotation, the list of users and who is active gets shifted unexpectedly. As a user, I would expect the current on-call person to stay on call.

Steps to Reproduce:

  1. Go to a rotation with a few users
  2. Click on 'Remove user' on someone before the active user
  3. Confirm, and notice the active user shift

Expected Behavior:

mastercactapus commented 6 months ago

I feel like we've had and fixed this bug before; fixing this should include a test for the regression

Forfold commented 2 months ago

Failing inside of updateRotationParticipants when calling updateRotation with a list of userIDs to remove:

    if len(userIDs) == 0 {
        // Delete rotation state if all users are going to be deleted as per new input
        err = m.RotationStore.DeleteStateTx(ctx, tx, rotationID)
        if err != nil {
            return err
        }
    } else if updateActive {
        // get current active participant
        s, err := m.RotationStore.StateTx(ctx, tx, rotationID)
        if errors.Is(err, rotation.ErrNoState) {
            return nil
        }
        if err != nil {
            return err
        }

        // if currently active user is going to be deleted
        // then set to first user before we actually delete any users
        if s.Position >= len(userIDs) {
            err = m.RotationStore.SetActiveIndexTx(ctx, tx, rotationID, 0)
            if err != nil {
                return err
            }
        }
    }