[BUG] Replacement player position overwritten to None

[UnravelSports]

Hi,

This issue is related to #411, but wanted to create a separate PR, because I don't know if this is the correct way to solve it.

Problem was that I got the below exception.

raise ValueError("Cannot create ranges when length < 2")

This happened when calling the MinutesPlayedAggregator.

The problem seemed to be that the below could not be constructed, because this replacement player had None for their position, and they only had 1 position.

for (
        start_time,
        end_time,
        position,
    ) in player.positions.ranges():

This meant that the below was bypassed, but we still ended up with only 1 item.

if self.items[items[-1]] is not None:
        current_period = items[0].period
        while current_period.next_period:
            current_period = current_period.next_period
        items.append(current_period.end_time)

Now, I think this happened because the same substitution event was processed 3 time, but I'm not 100% sure.
Because we set the event.player.set_position(event.time, None) after processing them to coming of the field, the second time we process this same substitution the replacement player get their position replaced by None as well.

def _update_formations_and_positions(self):
        """Update team formations and player positions based on Substitution and TacticalShift events."""
        max_leeway = timedelta(seconds=60)
        for event in self.events:
            
            if isinstance(event, SubstitutionEvent):
                if event.replacement_player.starting_position:
                    replacement_player_position = (
                        event.replacement_player.starting_position
                    )
                else:
                    replacement_player_position = event.player.positions.last(
                        default=PositionType.Unknown
                    )
                if event.replacement_player.positions.last(default=None) is None:
                    event.replacement_player.set_position(
                        event.time,
                        replacement_player_position,
                    )
                event.player.set_position(event.time, None)

Like I said, I don't know if this is the preferred way to resolve, maybe we should avoid parsing the same substitution multiple times, but this TimeContainer code is incredibly confusing, so I was just glad to find something that works for me.

[koenvo]

This fix seem to make sense. I’ve seen files before where a player was substituted and later on got back on the pitch. Probably an error in the data tho. Might be good to log a warning to indicate the error in the source file, instead of silently ignoring it. Nice opportunity to introduce a quality report some day :-)

Can you add a test for this?

[UnravelSports]

@koenvo @probberechts I totally missed your request to add a test.

I came back to this PR because I noticed another very small issue that is also related to corrupt data, but that can be solved relatively simply. Namely, player ids in the FormationChange event of a player that doesn't exist in the rest of the game. I don't know why this happens, but it's annoying to deal with.

Because I didn't know how to properly add a specific test for this I've instead updated the opta_f24.xml test file to include:

• A duplicate SubstitutionEvent
• A FormationChange event that includes a playerId (Id "1") that does not exist in the rest of the game.

Parsing the files now will not cause any issues and the tests all pass with the new changes.

I can understand that this indirect way of testing isn't optimal though, but is there a direct way of testing this?

fwiw, I also merged master.