Skip to content

fix(api+frontend): ensure storyboard has min. 1 section#2995

Merged
usu merged 10 commits into
ecamp:develfrom
usu:fix/storyboard-min-1-section
Oct 2, 2022
Merged

fix(api+frontend): ensure storyboard has min. 1 section#2995
usu merged 10 commits into
ecamp:develfrom
usu:fix/storyboard-min-1-section

Conversation

@usu

@usu usu commented Sep 25, 2022

Copy link
Copy Markdown
Member

Based on #2971

Show relevant diff only: https://github.com/ecamp/ecamp3/pull/2995/files/033f8f3a039f8f99a818a9a0d7c6faaeb4aecfcd..8262ae47a82ece54b897914472c8940e7add9d05

Fixes #2969

Fixes #2933

Only 1 section, delete button disabled

image

More than 1 section, dialog before section is removed

image

@usu usu changed the title Fix/storyboard min 1 section fix(api+frontend): ensure storyboard has min. 1 section Sep 25, 2022
@usu usu requested review from BacLuc and manuelmeister September 25, 2022 17:20

@BacLuc BacLuc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new StoryBoard is now not possible.
But you will fix that fast.


$this->assertCount(1, $response->toArray()['data']['sections']); // populated with 1 default section
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails now when you don't add the content as default of the field:

public function testCreateStoryboardAcceptsNotExistingJson() {
    $response = $this->create($this->getExampleWritePayload([], ['data']));

    $this->assertResponseStatusCodeSame(201);

    $this->assertCount(1, $response->toArray()['data']['sections']); // populated with 1 default section
}

Maybe set the value in the constructor of StoryBoard?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch. Constructor doesn't work (tried before), because in the constructor I don't know if the $data property will be populated later. That's why I put the default value into setData. Didn't recognize that setData is not called at all though if the property doesn't even exist in the payload.

Was anyway an odd design decision, so moved the default fallback now back into the data persister where it also should be.

Fixed with 0f70e58

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a similar case in the relation Camp -> Periods, where a camp always must have at least one period. How is this problem solved there?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There we enforce that the create payload contains a Period.
testCreateCampValidatesMissingPeriods

@usu usu requested review from carlobeltrame and pmattmann October 1, 2022 07:21
throw new UnexpectedTypeException($constraint, AssertJsonSchema::class);
}

if (null === $value) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wieso ist $value === null automatisch ok?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist ein Konzept was Symfony bei den eigenen Validators auch so einsetzt: null-Wert wird normalerweise als ok angesehen (bzw. vom Validator nicht überprüft) und man steuert separat via Assert\IsNull oder Assert\NotNull, wenn man null-Wert explizit vermeiden oder erzwingen will.

So is es nun möglich, dass wir null-Werte beim Erstellen erlauben, und in diesem Fall im DataPersister mit einem Default-Wert befüllen.

@usu usu merged commit c8bd269 into ecamp:devel Oct 2, 2022
@usu usu deleted the fix/storyboard-min-1-section branch November 6, 2022 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Json data: verify handling of empty objects ContentNode Storyboard: dialog to ask for confirmation before deletion of a section

4 participants