umbraco / Umbraco.Deploy.Issues

1 stars 0 forks source link

Decorator to inject transfer button doesn't work in infinite editing and trigger `GetById` multiple times #189

Open bjarnef opened 9 months ago

bjarnef commented 9 months ago

I noticed some issues with content resource GetById is being called when clicking "Queue for transfer" menu action on a node, but I wonder it this somehow affect clicking the info app?

Because each time I click the Info tab, I see a request to GetById but not in live environment, where "Queue for transfer" is not avaiable.

I noticed this because I have a dashboard and opening the content node in infinite mode cause 404 not found error, because of GetById= with empty id.

I moved the dashboard to CMS with latest changes in contrib branch, where I can't reproduce the issue.

image

image

Clicking info app on content node when id is present in route:

image

Clicking info app on content node in infinite editing where id isn't present in route:

image

I can reproduce this on two projects:

Umbraco 12.2.0 Deploy 12.0.5

Umbraco 12.3.3 Deploy 12.0.5


This item has been added to our backlog AB#35631

bjarnef commented 9 months ago

@AndyButland unfortunately I don't have access to source code of Deploy, but does Deploy listen to editors.apps.appChanged or call GetById somewhere - at least it does when clicking "Queue for transfer".

Can you reproduce the issue with requests to GetById on local or development environment when "Info" app is clicked or just each time switching app on content node?

AndyButland commented 9 months ago

I don't see any reference to appChanged and I can't see the issue with some quick checks of infinite editors (using the one when you look at relations on the "Info" tab for content or media). I think you'll need to give me step-by-step how to set up your content and/or dashboard to be able to replicate please (and also what version you are using). Thanks.

bjarnef commented 9 months ago

@AndyButland I have deployed the changed for this specific Umbraco Cloud project to live environment and I don't see the 404 errors from content nodes opened in infinite editing or the GetById requests clicking "Info" app. But I do get the errors on local and development environment.

I can invite you to the project if you want to have a look.

The versions are mentioned at bottom of the description. I can reproduce the issue on two Umbraco Cloud projects, but not on with latest changes in contrib branch in CMS repository. Even though I have implmented the exact same dashboard, which I think is weird.

bjarnef commented 9 months ago

From what I can tell the only difference on a content node between local, development and live environment on this Umbraco Cloud project, is that live environment doesn't have "Queue for transfer" action on content node.

AndyButland commented 9 months ago

Sure, invite me to the project and I'll have a look.

bjarnef commented 9 months ago

@AndyButland you should have received an invitation now.

bjarnef commented 9 months ago

Alternatively it could also be the "Compare" action, which also trigger GetById and only exists on local and development environment, but not on live environment.

When loading a content node I see two requests:

image

But on live environment only a single request:

image

bjarnef commented 9 months ago

I think it happens here:

image

This part won't work in infinite editing using $routeParams.id - and it probably shouldn't call this in infinite editing anyway since the actions are not available.

It should also happen only on load - not each time the an app change happen happen.

Let me know if I can help, but I know @mattbrailsford has worked a bit with the Angular decorators - otherwise I am sure the frontend team can assist.

bjarnef commented 9 months ago

Actually not related to "Compare" or "Queue for transfer", but the injected "Transfer now" button.

which isn't added to content editor in infinite editing.

image

because it used $routeParams.id

Perhaps it can used

var infiniteMode = $scope.model && $scope.model.infiniteMode;
var id = infiniteMode ? $scope.model.id : $routeParams.id;

but not sure about it in this context.

Altnatively something like this:

let infiniteMode = false;

// event for infinite editors
evts.push(eventsService.on("appState.editors.open", function (name, args) {
   infiniteMode  = args && args.editors.length > 0 ? true : false;
}));

evts.push(eventsService.on("appState.editors.close", function (name, args) {
   infiniteMode = args && args.editors.length > 0 ? true : false;
}));

https://github.com/umbraco/Umbraco-CMS/blob/c6d01178c06a519423b3c37fea7bbdd0e536319f/src/Umbraco.Web.UI.Client/src/main.controller.js#L209-L216

AndyButland commented 9 months ago

This looks tricky to properly resolve. Within this function, both $routeParams and editorState.getCurrent() refer to the item being edited under the infinite editor. And I can't inject $scope here it seems.

So I can't find a way to determine if I'm in "infinite" mode. If I could then, we could simply exit and not add the "Transfer Now" button.

I think I can live with the "Transfer Now" button not appearing when content is in the infinite editor. It doesn't really work anyway as it's trying to open a dialog under the infinite editor, and is in any referring to the wrong item.

So I'm thinking this change to ensure we don't try to add it if there's no id available.

if ($routeParams.section === "content" && $routeParams.id && $routeParams.create !== "true") {

Which would solve your problem. But there are still issues if you open up the content item say from the "Info" tab of another content item that has a relation with the first. There an id is available, but it's the "wrong" one.

bjarnef commented 9 months ago

@AndyButland it depends. Infine editing if often not the current node and id may not be present in route, e.g. from a dashboard.

Yes, the "Transfer now" button in infinite editing is not crucial. More important it doesn't include the double API request to GetById each time app button is clicked.

AndyButland commented 9 months ago

each time app button is clicked

Which "app button" are you referring to here?

bjarnef commented 9 months ago

The content app button like Info, Content etc. But also on initial page load. https://github.com/umbraco/Umbraco.Deploy.Issues/issues/189#issuecomment-1838070346

bjarnef commented 9 months ago

It seems the $delegate function should be configureContentEditorButtons instead of configureEditorButtons? https://github.com/umbraco/Umbraco-CMS/blob/941705b03de1d00676c2889c4438465e2709d55a/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js#L221

I also found and older example here: https://our.umbraco.com/forum/umbraco-7/developing-umbraco-7-packages/53758-Catch-content-page-load-event-in-umbraco-704

bjarnef commented 9 months ago

Maybe an option could be to inject $location can check if path property contains edit?

However I think the way the decorator is contructured in Deploy, it trigger multiple GetById requests, which it shouldn't.

I tested with this for instance:

angular.module('umbraco.services').config([
    '$provide',
    function ($provide) {

        $provide.decorator('contentEditingHelper', [
            "$delegate", "$routeParams", "$rootScope", "$location", "editorState", "localizationService", "navigationService",
            function ($delegate, $routeParams, $rootScope, $location, editorState, localizationService, navigationService) {

                console.log("$delegate", $delegate);
                console.log("$routeParams", $routeParams);
                console.log("$rootScope", $rootScope);
                console.log("$location", $location);
                console.log("editorState", editorState);

                $delegate.configureContentEditorButtons = (function () {
                    var cached_function = $delegate.configureContentEditorButtons;

                    console.log("cached_function", cached_function);

                    function addCustomButtonToButtonSet(buttons, node, localizationService, navigationService) {
                        addButtonToButtonSet(buttons, node, localizationService, navigationService,
                            "D", "Hello world!", "ctrl+d", null);
                    }

                    function addAnotherCustomButtonToButtonSet(buttons, node, localizationService, navigationService) {
                        addButtonToButtonSet(buttons, node, localizationService, navigationService,
                            "D", "Hi Andy!", "ctrl+d", null);
                    }

                    function addButtonToButtonSet(buttons, node, localizationService, navigationService, letter, label, labelKey, hotKey, dialogView) {
                        buttons.push({
                            letter: letter,
                            label: label,
                            //labelKey: "actions_" + labelKey,
                            hotKey: hotKey,
                            hotKeyWhenHidden: true,
                            handler: function () {

                                //getting the current tree node to open the dialog against.
                                //if (!node.nodeType && node.udi) {
                                //    node.nodeType = getEntityTypeFromUdi(node.udi);
                                //}

                                //localizationService.localize("dialogs_" + labelKey + "Title").then(function (value) {
                                //    navigationService.showDialog({
                                //        action: {
                                //            name: value,
                                //            metaData: {
                                //                actionView: "../App_Plugins/Deploy/views/dialogs/" + dialogView,
                                //                dialogMode: true
                                //            }
                                //        },
                                //        node: node
                                //    });
                                //});
                            }
                        });
                    };

                    return function () {
                        var buttons = cached_function.apply(this, arguments);

                        console.log("buttons", buttons);

                        const path = $location.path();
                        console.log("path", $location.path());

                        if ($routeParams.section === "content" && $routeParams.create !== "true" && path.split("/").includes("edit")) {

                            addCustomButtonToButtonSet(buttons.subButtons, editorState.current, localizationService, navigationService);

                        }
                        else {
                            addAnotherCustomButtonToButtonSet(buttons.subButtons, editorState.current, localizationService, navigationService);
                        }

                        //if (Umbraco.Sys.ServerVariables.deploy &&
                        //    Umbraco.Sys.ServerVariables.deploy.AllowDeployOptions &&
                        //    Umbraco.Sys.ServerVariables.deploy.AllowDeployOptions === true) {

                        //    if ($routeParams.section === "content" && $routeParams.create !== "true") {

                        //        // Only add the "transfer now" button if the user has permissions to "queue for transfer".
                        //        contentResource.getById($routeParams.id).then(function (content) {
                        //            if (_.contains(content.allowedActions, "T")) {

                        //            }
                        //        });
                        //    }
                        //}
                        return buttons;
                    };
                }());

                return $delegate;
            }]);

    }]);

On content edit page

image

On content edit page in infinite editing

image

bjarnef commented 9 months ago

Instead of fetching content again using GetById to get allowed actions and if we can get content from editorState, then we can listen to some of these events instead, e.g. content.loaded: https://github.com/umbraco/Umbraco-CMS/blob/941705b03de1d00676c2889c4438465e2709d55a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js#L250

eventsService.on("content.loaded", (evt, data) => {

    console.log("content.loaded", evt, data);

    const content = data.content;

    console.log("allowedActions", content.allowedActions);
});

image

Howeever shouldn't it also be possible to get content from args, where we override/customize the default function: https://github.com/umbraco/Umbraco-CMS/blob/941705b03de1d00676c2889c4438465e2709d55a/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js#L226