Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-29979: As a developer I would like to use getLocation method on ContentCreateSuccessView and ContentEditSuccessView #270

Merged
merged 5 commits into from Mar 4, 2019

Conversation

kmadejski
Copy link
Member

…ontentCreateSuccessView and ContentEditSuccessView
lib/Content/View/ContentCreateSuccessView.php Outdated Show resolved Hide resolved
lib/Content/View/ContentEditSuccessView.php Outdated Show resolved Hide resolved
webhdx and others added 2 commits January 7, 2019 12:15
Co-Authored-By: kmadejski <kmadejski@live.com>
Co-Authored-By: kmadejski <kmadejski@live.com>
@mnocon mnocon self-assigned this Jan 9, 2019
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Sadly, it breaks when trying to publish a previously saved draft.
I've used dev env with debug enabled.

Steps:

  1. Login as admin
  2. Go to Content
  3. Start creating a Folder (enter "TestName" as name)
  4. Save draft
  5. Publish

It fails with:

Type error: Argument 1 passed to EzSystems\RepositoryForms\Content\View\ContentEditSuccessView::setLocation() must be an instance of eZ\Publish\API\Repository\Values\Content\Location, null given, called in /home/vagrant/ee_demo/vendor/ezsystems/repository-forms/lib/Content/View/Builder/ContentEditViewBuilder.php on line 126

I believe we shouldn't set the location to null there - it should be the location that was created when publish action was executed? (If it's possible).

@kmadejski
Copy link
Member Author

@andrerom / @ViniTou I have to allow passing null as $location due to the issue found by QA.

@mnocon could you please retest?

@kmadejski
Copy link
Member Author

@mnocon do you see any chance to give it another round of testing? 🙂

@mnocon
Copy link
Member

mnocon commented Feb 27, 2019

@kmadejski there is a chance 😉 But you're probably aware that 60% of our QA team is on sick leave and it complicates things a bit

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

I know that the ticket mentions ContentCreateSuccessView and ContentEditSuccessView only (and it works correctly for them), but wouldn't it make sense to implement LocationValueView also for EzSystems\EzPlatformPageBuilder\View\PageView, EzSystems\EzPlatformFormBuilder\View\FieldConfigurationView and EzSystems\RepositoryForms\User\View\UserUpdateView?

This way we would provide unified experience for developers across all these editing views.

Also, ping @DominikaK - not sure if it's something that is doc-worthy.

@kmadejski
Copy link
Member Author

Good question @mnocon, honestly, I don't know. @ViniTou / @webhdx / @Nattfarinn / @andrerom
WDYT?

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Discussed with Kamil - the rest of the Views can be changed in a follow-up ticket (if decided so).

@lserwatka lserwatka merged commit 89301fc into ezsystems:master Mar 4, 2019
ViniTou pushed a commit to ViniTou/repository-forms that referenced this pull request Apr 29, 2019
…ontentCreateSuccessView and ContentEditSuccessView (ezsystems#270)

* EZP-29979: As a developer I would like to use getLocation method on ContentCreateSuccessView and ContentEditSuccessView

* Removed null from setLocation

* Update lib/Content/View/ContentCreateSuccessView.php

Co-Authored-By: kmadejski <kmadejski@live.com>

* Update lib/Content/View/ContentEditSuccessView.php

Co-Authored-By: kmadejski <kmadejski@live.com>

* Recovered possiblity to pass null to setLocation method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants