Fix unintended side effect of add_cmds

[taldcroft]

Description

The CommandTable.add_cmds() method had a bug where it would update the original self table in place (using remove_rows) prior to using it for computing the new table with additional commands. I think the original idea of this method was for the whole thing to be in-place but that was inconvenient.

This fixes the issue by not using remove_rows and defines a new reference to the original commands which is filtered NOT in-place.

This also adds a careful test of the method which was previously lacking.

Interface impacts

None.

Testing

Unit tests

• Mac

(masters) ➜  kadi git:(add-cmds-side-effect-bug) git rev-parse HEAD           
b18bd8c8916651397b313748a1bb7d30f1c8c28c
(masters) ➜  kadi git:(add-cmds-side-effect-bug) pytest                       
========================================================= test session starts =========================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 229 items                                                                                                                   

kadi/commands/tests/test_commands.py .....................................................................................      [ 37%]
kadi/commands/tests/test_states.py ......................x.............................................x....................... [ 77%]
                                                                                                                                [ 77%]
kadi/commands/tests/test_validate.py ....................                                                                       [ 86%]
kadi/tests/test_events.py ..........                                                                                            [ 90%]
kadi/tests/test_occweb.py ......................                                                                                [100%]

============================================= 227 passed, 2 xfailed in 143.25s (0:02:23) ==============================================

Independent check of unit tests by [REVIEWER NAME]

• [PLATFORM]:

Functional tests

No functional testing.

[javierggt]

I reviewed the changes and they make sense to me. I read the test in detail, and verified that the test passes in the branch and fails in master.

While reading the test I noticed that the commands that are clipped are the ones with times after the RLTT, which was not obvious to me before. The docstring does not specify that, and instead uses colloquial language (Clip existing commands to the RLTT). It would have been nice to have it in the docstring).

[taldcroft]

The docstring does not specify that, and instead uses colloquial language (Clip existing commands to the RLTT). It would have been nice to have it in the docstring).

I'll improve the docstring, that's a good point. There is already some PR cross-talk with the docstring here so I don't want to touch this now, praying to not generate merge conflicts when this gets merged.