Shadowing: use DTO's #2331

nickygerritsen · 2024-02-14T20:33:27Z

This should have (almost) no functional changes, but it uses DTO's for reading from the event feed, making it that we can get rid of a lot of array shapes and that we get added type safety.

I did add the provider object to the API root endpoint, so we can reuse it here and added some small improvements, see the separate commits.

webapp/src/Serializer/Shadowing/EventDenormalizer.php

webapp/src/Controller/API/GeneralInfoController.php

webapp/src/DataTransferObject/Shadowing/GroupEvent.php

webapp/src/DataTransferObject/Shadowing/OrganizationEvent.php

webapp/src/DataTransferObject/Shadowing/ProblemEvent.php

webapp/src/Service/ExternalContestSourceService.php

webapp/src/DataTransferObject/Shadowing/ContestData.php

vmcj

Good enough to merge as is, there are some discussion points left for future PRs.

meisterT · 2024-02-23T17:56:33Z

webapp/src/DataTransferObject/Shadowing/Award.php

+
+class Award implements EventData
+{
+    // Not used currently, so no fields


why does the class exist then?

Otherwise we need special checks for unknown events. This makes the code a bit cleaner

Would you prefer to add the special cases to the ExternalContestSourceService?

meisterT · 2024-02-23T17:57:29Z

webapp/src/DataTransferObject/Shadowing/ContestData.php

+        public readonly ?string $scoreboardFreezeDuration,
+        public readonly int $penaltyTime,
+        public readonly ?string $startTime,
+        // TODO: check for end time and scoreboard thaw time


Probably want to resolve before merging?

Nope. @vmcj wanted this comment. It would be a new “feature”

meisterT · 2024-02-23T17:58:50Z

webapp/src/DataTransferObject/Shadowing/TeamMember.php

+
+class TeamMember implements EventData
+{
+    // Not used currently, so no fields


same as above, is it necessary?

eldering · 2024-02-24T12:59:48Z

webapp/src/Service/ExternalContestSourceService.php

+        bool $fromStart,
+        array $eventsToSkip,
+        ?callable $progressReporter = null
+    ): bool {


I see this is the same change as in #2343 so I suppose that PR indeed contains duplicate commits.

Also, like my comment there: can we please not do reformatting of code that is not otherwise touched by your changes.

My bad. I am very used to (at work) reformat a file with a code style before saving to make it in sync.
I will try to undo all these reformat changes (even though they don't adhere to the code style).

eldering · 2024-02-24T13:02:43Z

webapp/src/Service/ExternalContestSourceService.php

+            $freezeHour = floor($freezeStartSeconds / 3600);
+            $freezeMinutes = floor(($freezeStartSeconds % 3600) / 60);
+            $freezeSeconds = floor(($freezeStartSeconds % 60) / 60);
+            $freezeMilliseconds = $freezeStartSeconds - floor($freezeStartSeconds);


I disagree with this kind of removal of alignment spaces. It makes the code less readable.

My bad, this is what the code style currently says. I will undo these changes.

I don't know exactly what the current code style says, but I strongly think that automatic reformatting of alignment like this should be removed from it. Code formatting is a means to an end (better readability of code) and not a goal in itself.

I believe the problem is it wants some alignment and you can’t say “do whatever there was” but I hope I’m wrong since that would save me time

vmcj · 2024-02-25T00:05:26Z

webapp/src/Serializer/Shadowing/EventDenormalizer.php

+
+        $eventType = EventType::fromString($data['type']);
+        if ($this->getEventFeedFormat($data, $context) === EventFeedFormat::Format_2022_07) {
+            $operation = !isset($data['data']) ? Operation::DELETE : Operation::CREATE;


Suggested change

$operation = !isset($data['data']) ? Operation::DELETE : Operation::CREATE;

$operation = isset($data['data']) ? Operation::CREATE : Operation::DELETE;

What is the reason for this?

it's more readable?

nickygerritsen · 2024-02-25T19:23:45Z

@vmcj found an issue with legacy delete events. I will fix that but before doing that add tests for some events so we know they are parsed correctly.

vmcj · 2024-02-25T20:29:27Z

webapp/src/Service/ExternalContestSourceService.php

    {
-        $hash       = ExternalSourceWarning::calculateHash($type, $entityType, $entityId);
-        $warning    = $this->em
+        $hash = ExternalSourceWarning::calculateHash($type, $eventType->value, $entityId);


Not sure if you touch this in your PR or if it was already broken but calculateHash expects a string for $entityId but that can be null

…rvice.

…ontest sources.

Co-authored-by: MCJ Vasseur <[email protected]>

Also add comment to event classes that are unused.

Also fix some logic errors.

vmcj reviewed Feb 14, 2024

View reviewed changes

webapp/src/Serializer/Shadowing/EventDenormalizer.php Show resolved Hide resolved

nickygerritsen changed the title ~~Shadow differences: redirect to configure page if not set up yet.~~ Shadowing: use DTO's Feb 15, 2024