Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

EZP-28003: Unable to go back to the Content structure after content move to the Media library #913

Merged
merged 1 commit into from
Oct 23, 2017
Merged

EZP-28003: Unable to go back to the Content structure after content move to the Media library #913

merged 1 commit into from
Oct 23, 2017

Conversation

sunpietro
Copy link
Contributor

@sunpietro sunpietro added the Bug label Oct 17, 2017
@sunpietro sunpietro self-assigned this Oct 17, 2017
@sunpietro sunpietro changed the base branch from master to 1.11 October 17, 2017 10:20
@sunpietro
Copy link
Contributor Author

Just one comment from my side:

I've used window.location.replace instead of window.history.pushState because I wanted to remove the incorrect location of content from the browser history. pushState adds a new entry a last visited page, but you can still get back to the incorrect location.

@ezsystems ezsystems deleted a comment from ezrobot Oct 17, 2017
@ezsystems ezsystems deleted a comment from ezrobot Oct 17, 2017
@ezsystems ezsystems deleted a comment from ezrobot Oct 17, 2017
kmadejski
kmadejski suggested changes Oct 17, 2017
View reviewed changes
Copy link
Member

@kmadejski kmadejski left a comment

Choose a reason for hiding this comment

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

@sunpietro The bug with back button is solved in fact, but the issue with the second way to switch tab still exists.
After you moved the image to the Media -> Images you can use back button, but when you click on Content structure (currently, you should be under Media library) then you got the same exception and you have to reload the whole application.

dew326
dew326 suggested changes Oct 17, 2017
View reviewed changes
Resources/public/js/views/services/ez-locationviewviewservice.js
@@ -2,6 +2,7 @@
* Copyright (C) eZ Systems AS. All rights reserved.
* For full copyright and license information view LICENSE file distributed with this source code.
*/
/*jshint esversion: 6 */
Copy link
Member

Choose a reason for hiding this comment

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

It will be better to add this into config not per file

Resources/public/js/views/services/ez-locationviewviewservice.js Outdated
* @method _updatePrevHistory
* @protected
* @param {any} location
* @param {any} callback
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not any.

Resources/public/js/views/services/ez-locationviewviewservice.js Outdated
_updatePrevHistory(location, callback) {
const urlPart = window.decodeURIComponent(window.location.hash);
const language = urlPart.slice(urlPart.lastIndexOf('/'), urlPart.length);
const hashStart = '#/view/';
Copy link
Member

Choose a reason for hiding this comment

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

Can't we avoid hardcoded parts?

@ezsystems ezsystems deleted a comment from ezrobot Oct 17, 2017
@ezsystems ezsystems deleted a comment from ezrobot Oct 19, 2017
@sunpietro
Copy link
Contributor Author

ping @kmadejski @dew326

dew326
dew326 suggested changes Oct 19, 2017
View reviewed changes
Resources/public/js/views/services/ez-navigationhubviewservice.js Outdated
* @param {Event} event
*/
_resetContentNavigationItem: function () {
const contentItem = this.get('platformNavigationItems').find(item => {
Copy link
Member

Choose a reason for hiding this comment

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

I think method getNavigationItem do the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but this one is faster because I'm looking for a specific item in a specific zone. I don't have to iterate through all the zones to get that one specific item :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree it's faster around 0.0002ms :D
I think we should use dedicated methods for finding items if such method exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can agree to this option.

@ezsystems ezsystems deleted a comment from ezrobot Oct 19, 2017
@lserwatka
Copy link
Member

@kmadejski Could you help test this?

@kmadejski
Copy link
Member

@sunpietro works well. Just only one thing, when I moved content I got an exception, to be more precise I got the only red notification on the bottom. There is no error in console and no invalid request in Network tab (in developer tools).

screen shot 2017-10-20 at 14 39 26

@dew326
Copy link
Member

dew326 commented Oct 20, 2017

I think it's something from the StudioUI. Do you have Platform or Platform EE?

@sunpietro
Copy link
Contributor Author

If you have eZPlatform-EE then you definitely need the code from the PR I referenced above.
Please, update your studio-ui-bundle and use the branch from PR

@kmadejski
Copy link
Member

kmadejski commented Oct 20, 2017 •
edited
Loading

@sunpietro I apologize, I overlooked this reference. I patched also StudioUI bundle, and yes, the previous message doesn't appear anymore, but I've got another "red" message (no Console/Network errors). I'm getting this message right after entry to the Content structure tab.

screen shot 2017-10-20 at 15 17 28

@dew326 it is Platform EE 1.11.

@sunpietro
Copy link
Contributor Author

@kmadejski I'm on it

@kmadejski
Copy link
Member

@sunpietro after update with https://github.com/ezsystems/StudioUIBundle/pull/852 everything works well!

@dew326
Copy link
Member

dew326 commented Oct 23, 2017

Question: Isn't it a BC break if changes in PlatformUI requires additional work in StudioUI?

@sunpietro
Copy link
Contributor Author

@dew326 it's not BC break. PlatformUI didn't have any functionality to correct the history of previously visited pages after moving a content item to a new location. This fix corrects this behaviour.
The StudioUI relies on a locations previewed in the Content Structure. If the Content Structure navigation item points to an incorrect location, then moving from Content Structure to the Page Mode will cause errors. The error existed before and now it's fixed.

@lserwatka
Copy link
Member

@micszo we need a QA here + corresponding Studio PR.

@micszo
Copy link
Member

micszo commented Oct 23, 2017

@lserwatka OK, it's top on the list now.

micszo
micszo approved these changes Oct 23, 2017
View reviewed changes
Copy link
Member

@micszo micszo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Retest OK with eZ Platform 1.11.0.3 and eZ Platform EE 1.11.0.
Please attach patch.

@lserwatka lserwatka merged commit 66b32a2 into ezsystems:1.11 Oct 23, 2017
@sunpietro sunpietro deleted the ezp-28003-fix-previous-location-after-moving-content branch October 23, 2017 12:03
@sunpietro sunpietro mentioned this pull request Dec 22, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kmadejski kmadejski kmadejski approved these changes

@dew326 dew326 dew326 approved these changes

@micszo micszo micszo approved these changes

@lserwatka lserwatka Awaiting requested review from lserwatka

@mnocon mnocon Awaiting requested review from mnocon

Assignees

@sunpietro sunpietro

Labels
Bug
Milestone
No milestone
5 participants
@sunpietro @lserwatka @kmadejski @dew326 @micszo