From 66a00546d141d712a410b86a68ae2a3bc7e60a68 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 14 Aug 2018 23:19:32 +1000 Subject: [PATCH] more work with saving, publishing and validation for variants, fixes up a few old things too --- .../Migrations/Install/DatabaseDataCreator.cs | 2 +- .../Persistence/Factories/PropertyFactory.cs | 4 + .../components/content/edit.controller.js | 41 +--- .../services/contenteditinghelper.service.js | 176 ++++++++++-------- .../src/common/services/formhelper.service.js | 3 - .../content/overlays/publish.controller.js | 35 ++-- .../views/content/overlays/save.controller.js | 30 +-- .../Filters/ContentItemValidationHelper.cs | 10 + 8 files changed, 140 insertions(+), 161 deletions(-) diff --git a/src/Umbraco.Core/Migrations/Install/DatabaseDataCreator.cs b/src/Umbraco.Core/Migrations/Install/DatabaseDataCreator.cs index e7a7296596..be9a1b9b74 100644 --- a/src/Umbraco.Core/Migrations/Install/DatabaseDataCreator.cs +++ b/src/Umbraco.Core/Migrations/Install/DatabaseDataCreator.cs @@ -232,7 +232,7 @@ namespace Umbraco.Core.Migrations.Install private void CreateLanguageData() { - _database.Insert(Constants.DatabaseSchema.Tables.Language, "id", false, new LanguageDto { Id = 1, IsoCode = "en-US", CultureName = "en-US" }); + _database.Insert(Constants.DatabaseSchema.Tables.Language, "id", false, new LanguageDto { Id = 1, IsoCode = "en-US", CultureName = "English (United States)" }); } private void CreateContentChildTypeData() diff --git a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs index 3bea84e619..c920a18c3b 100644 --- a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs @@ -27,7 +27,11 @@ namespace Umbraco.Core.Persistence.Factories if (xdtos.TryGetValue(propertyType.Id, out var propDtos)) { foreach (var propDto in propDtos) + { + property.Id = propDto.Id; property.FactorySetValue(languageRepository.GetIsoCodeById(propDto.LanguageId), propDto.Segment, propDto.VersionId == publishedVersionId, propDto.Value); + } + } property.ResetDirtyProperties(false); diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js index 24ac3b9cea..67bef8a2ff 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js @@ -306,7 +306,8 @@ saveMethod: args.saveMethod, scope: $scope, content: $scope.content, - action: args.action + action: args.action, + showNotifications: args.showNotifications }).then(function (data) { //success init($scope.content); @@ -441,7 +442,8 @@ //we need to return this promise so that the dialog can handle the result and wire up the validation response return performSave({ saveMethod: contentResource.publish, - action: "publish" + action: "publish", + showNotifications: false }).then(function (data) { overlayService.close(); return $q.when(data); @@ -451,21 +453,6 @@ //re-map the dialog model since we've re-bound the properties dialog.variants = $scope.content.variants; - //check the error list for specific variant errors, if none exist that means that only server side validation - //for the current variant's properties failed, in this case we want to close the publish dialog since the user - //will need to fix validation errors on the properties - if (err.data && err.data.ModelState) { - var keys = _.keys(err.data.ModelState); - var foundVariantError = _.find(keys, - function (k) { - return k.startsWith("_content_variant_"); - }); - if (!foundVariantError) { - //no variant errors, close the dialog - overlayService.close(); - } - } - return $q.reject(err); }); }, @@ -500,7 +487,8 @@ //we need to return this promise so that the dialog can handle the result and wire up the validation response return performSave({ saveMethod: $scope.saveMethod(), - action: "save" + action: "save", + showNotifications: false }).then(function (data) { overlayService.close(); return $q.when(data); @@ -509,22 +497,7 @@ model.submitButtonState = "error"; //re-map the dialog model since we've re-bound the properties dialog.variants = $scope.content.variants; - - //check the error list for specific variant errors, if none exist that means that only server side validation - //for the current variant's properties failed, in this case we want to close the publish dialog since the user - //will need to fix validation errors on the properties - if (err.data && err.data.ModelState) { - var keys = _.keys(err.data.ModelState); - var foundVariantError = _.find(keys, - function (k) { - return k.startsWith("_content_variant_"); - }); - if (!foundVariantError) { - //no variant errors, close the dialog - overlayService.close(); - } - } - + return $q.reject(err); }); }, diff --git a/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js b/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js index 11bae4e027..211db9f267 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/contenteditinghelper.service.js @@ -7,19 +7,19 @@ **/ function contentEditingHelper(fileManager, $q, $location, $routeParams, notificationsService, navigationService, localizationService, serverValidationManager, dialogService, formHelper, appState) { - function isValidIdentifier(id){ + function isValidIdentifier(id) { //empty id <= 0 - if(angular.isNumber(id) && id > 0){ + if (angular.isNumber(id) && id > 0) { return true; } //empty guid - if(id === "00000000-0000-0000-0000-000000000000"){ + if (id === "00000000-0000-0000-0000-000000000000") { return false; } //empty string / alias - if(id === ""){ + if (id === "") { return false; } @@ -44,6 +44,9 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica if (!args.saveMethod) { throw "args.saveMethod is not defined"; } + if (args.showNotifications === undefined) { + args.showNotifications = true; + } var redirectOnSuccess = args.redirectOnSuccess !== undefined ? args.redirectOnSuccess : true; var redirectOnFailure = args.redirectOnFailure !== undefined ? args.redirectOnFailure : true; @@ -66,7 +69,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica scope: args.scope, savedContent: data, redirectOnSuccess: redirectOnSuccess, - rebindCallback: function() { + rebindCallback: function () { rebindCallback.apply(self, [args.content, data]); } }); @@ -76,13 +79,14 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica }, function (err) { self.handleSaveError({ + showNotifications: args.showNotifications, redirectOnFailure: redirectOnFailure, err: err, - rebindCallback: function() { + rebindCallback: function () { rebindCallback.apply(self, [args.content, err.data]); } }); - + args.scope.busy = false; return $q.reject(err); }); @@ -90,9 +94,9 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica else { return $q.reject(); } - + }, - + /** Used by the content editor and media editor to add an info tab to the tabs array (normally known as the properties tab) */ addInfoTab: function (tabs) { @@ -165,7 +169,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica handler: args.methods.sendToPublish, hotKey: "ctrl+p", hotKeyWhenHidden: true, - alias: "sendToPublish" + alias: "sendToPublish" }; case "A": //save @@ -175,7 +179,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica handler: args.methods.save, hotKey: "ctrl+s", hotKeyWhenHidden: true, - alias: "save" + alias: "save" }; case "Z": //unpublish @@ -185,7 +189,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica handler: args.methods.unPublish, hotKey: "ctrl+u", hotKeyWhenHidden: true, - alias: "unpublish" + alias: "unpublish" }; default: return null; @@ -247,15 +251,15 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica // If we have a scheduled publish or unpublish date change the default button to // "save" and update the label to "save and schedule - if(args.content.releaseDate || args.content.removeDate) { + if (args.content.releaseDate || args.content.removeDate) { // if save button is alread the default don't change it just update the label if (buttons.defaultButton && buttons.defaultButton.letter === "A") { buttons.defaultButton.labelKey = "buttons_saveAndSchedule"; return; } - - if(buttons.defaultButton && buttons.subButtons && buttons.subButtons.length > 0) { + + if (buttons.defaultButton && buttons.subButtons && buttons.subButtons.length > 0) { // save a copy of the default so we can push it to the sub buttons later var defaultButtonCopy = angular.copy(buttons.defaultButton); var newSubButtons = []; @@ -313,66 +317,66 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica * @description * Returns a letter array for buttons, with the primary one first based on content model, permissions and editor state */ - getAllowedActions : function(content, creating){ + getAllowedActions: function (content, creating) { - //This is the ideal button order but depends on circumstance, we'll use this array to create the button list - // Publish, SendToPublish, Save - var actionOrder = ["U", "H", "A"]; - var defaultActions; - var actions = []; + //This is the ideal button order but depends on circumstance, we'll use this array to create the button list + // Publish, SendToPublish, Save + var actionOrder = ["U", "H", "A"]; + var defaultActions; + var actions = []; - //Create the first button (primary button) - //We cannot have the Save or SaveAndPublish buttons if they don't have create permissions when we are creating a new item. - if (!creating || _.contains(content.allowedActions, "C")) { - for (var b in actionOrder) { - if (_.contains(content.allowedActions, actionOrder[b])) { - defaultAction = actionOrder[b]; - break; - } + //Create the first button (primary button) + //We cannot have the Save or SaveAndPublish buttons if they don't have create permissions when we are creating a new item. + if (!creating || _.contains(content.allowedActions, "C")) { + for (var b in actionOrder) { + if (_.contains(content.allowedActions, actionOrder[b])) { + defaultAction = actionOrder[b]; + break; + } + } + } + + actions.push(defaultAction); + + //Now we need to make the drop down button list, this is also slightly tricky because: + //We cannot have any buttons if there's no default button above. + //We cannot have the unpublish button (Z) when there's no publish permission. + //We cannot have the unpublish button (Z) when the item is not published. + if (defaultAction) { + //get the last index of the button order + var lastIndex = _.indexOf(actionOrder, defaultAction); + + //add the remaining + for (var i = lastIndex + 1; i < actionOrder.length; i++) { + if (_.contains(content.allowedActions, actionOrder[i])) { + actions.push(actionOrder[i]); } } - actions.push(defaultAction); - - //Now we need to make the drop down button list, this is also slightly tricky because: - //We cannot have any buttons if there's no default button above. - //We cannot have the unpublish button (Z) when there's no publish permission. - //We cannot have the unpublish button (Z) when the item is not published. - if (defaultAction) { - //get the last index of the button order - var lastIndex = _.indexOf(actionOrder, defaultAction); - - //add the remaining - for (var i = lastIndex + 1; i < actionOrder.length; i++) { - if (_.contains(content.allowedActions, actionOrder[i])) { - actions.push(actionOrder[i]); - } - } - - //if we are not creating, then we should add unpublish too, - // so long as it's already published and if the user has access to publish - if (!creating) { - if (content.publishDate && _.contains(content.allowedActions,"U")) { - actions.push("Z"); - } + //if we are not creating, then we should add unpublish too, + // so long as it's already published and if the user has access to publish + if (!creating) { + if (content.publishDate && _.contains(content.allowedActions, "U")) { + actions.push("Z"); } } + } - return actions; - }, + return actions; + }, - /** - * @ngdoc method - * @name umbraco.services.contentEditingHelper#getButtonFromAction - * @methodOf umbraco.services.contentEditingHelper - * @function - * - * @description - * Returns a button object to render a button for the tabbed editor - * currently only returns built in system buttons for content and media actions - * returns label, alias, action char and hot-key - */ - getButtonFromAction : function(ch){ + /** + * @ngdoc method + * @name umbraco.services.contentEditingHelper#getButtonFromAction + * @methodOf umbraco.services.contentEditingHelper + * @function + * + * @description + * Returns a button object to render a button for the tabbed editor + * currently only returns built in system buttons for content and media actions + * returns label, alias, action char and hot-key + */ + getButtonFromAction: function (ch) { switch (ch) { case "U": return { @@ -407,7 +411,8 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica return null; } - }, + }, + /** * @ngdoc method * @name umbraco.services.contentEditingHelper#reBindChangedProperties @@ -421,11 +426,11 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica * This returns the list of changed content properties (does not include standard object property changes). */ reBindChangedProperties: function (origContent, savedContent) { - + //TODO: We should probably split out this logic to deal with media/members seperately to content - + //a method to ignore built-in prop changes - var shouldIgnore = function(propName) { + var shouldIgnore = function (propName) { return _.some([ "variants", "notifications", @@ -465,9 +470,12 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica //Now re-bind content properties. Since content has variants and media/members doesn't, //we'll detect the variants property for content to distinguish if it's content vs media/members. + var isContent = false; + var origVariants = []; var savedVariants = []; if (origContent.variants) { + isContent = true; //it's contnet so assign the variants as they exist origVariants = origContent.variants; savedVariants = savedContent.variants; @@ -492,6 +500,12 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica var origVariant = origVariants[j]; var savedVariant = savedVariants[j]; + //special case for content, don't sync this variant if it wasn't tagged + //for saving in the first place + if (!savedVariant.save) { + continue; + } + //if it's content (not media/members), then we need to sync the variant specific data if (origContent.variants) { @@ -501,7 +515,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica //loop through the properties returned on the server object for (var b in savedVariant) { - var shouldCompare = _.some(variantPropertiesSync, function(e) { + var shouldCompare = _.some(variantPropertiesSync, function (e) { return e === b; }); @@ -528,7 +542,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica //they have changed so set the origContent prop to the new one var origVal = origProp.value; - + origProp.value = newProp.value; //instead of having a property editor $watch their expression to check if it has @@ -539,7 +553,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica origProp.onValueChanged(origProp.value, origVal); } - changed.push(origProp); + changed.push(origProp); } } @@ -577,6 +591,13 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica //wire up the server validation errs formHelper.handleServerValidation(args.err.data.ModelState); + //add model state errors to notifications + if (args.showNotifications) { + for (var e in modelState) { + notificationsService.error("Validation", modelState[e][0]); + } + } + if (!args.redirectOnFailure || !this.redirectToCreatedContent(args.err.data.id, args.err.data.ModelState)) { //we are not redirecting because this is not new content, it is existing content. In this case // we need to detect what properties have changed and re-bind them with the server data. Then we need @@ -621,7 +642,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica // the default behaviour is to redirect on success. This adds option to prevent when false args.redirectOnSuccess = args.redirectOnSuccess !== undefined ? args.redirectOnSuccess : true; - + if (!args.redirectOnSuccess || !this.redirectToCreatedContent(args.redirectId ? args.redirectId : args.savedContent.id)) { //we are not redirecting because this is not new content, it is existing content. In this case @@ -646,9 +667,8 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica */ redirectToCreatedContent: function (id, modelState) { - //only continue if we are currently in create mode and not in infinite mode and if there is no 'Name' modelstate errors - // since we need at least a name to create content. - if ($routeParams.create && (isValidIdentifier(id) && (!modelState || !modelState["Name"]))) { + //only continue if we are currently in create mode and not in infinite mode and if the resulting ID is valid + if ($routeParams.create && (isValidIdentifier(id))) { //need to change the location to not be in 'create' mode. Currently the route will be something like: // /belle/#/content/edit/1234?doctype=newsArticle&create=true @@ -659,7 +679,7 @@ function contentEditingHelper(fileManager, $q, $location, $routeParams, notifica navigationService.clearSearch(); //change to new path - $location.path("/" + $routeParams.section + "/" + $routeParams.tree + "/" + $routeParams.method + "/" + id); + $location.path("/" + $routeParams.section + "/" + $routeParams.tree + "/" + $routeParams.method + "/" + id); //don't add a browser history for this $location.replace(); return true; diff --git a/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js b/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js index 3a5dfe28de..c019258a02 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/formhelper.service.js @@ -190,9 +190,6 @@ function formHelper(angularHelper, serverValidationManager, $timeout, notificati serverValidationManager.addFieldError(e, modelState[e][0]); } - //add to notifications - notificationsService.error("Validation", modelState[e][0]); - } } }; diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js index 1667b60d43..6d9c8303b4 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js @@ -4,32 +4,13 @@ function PublishController($scope, localizationService) { var vm = this; - vm.variants = $scope.model.variants; - vm.changeSelection = changeSelection; vm.loading = true; - vm.dirtyVariantFilter = dirtyVariantFilter; - vm.pristineVariantFilter = pristineVariantFilter; vm.hasPristineVariants = false; - //watch this model, if it's reset, then re init - $scope.$watch("model.variants", - function (newVal, oldVal) { - vm.variants = newVal; - if (oldVal && oldVal.length) { - //re-bind the selections - for (var i = 0; i < oldVal.length; i++) { - var found = _.find(vm.variants, function (v) { - return v.language.id === oldVal[i].language.id; - }); - if (found) { - found.publish = oldVal[i].publish; - } - - } - } - onInit(); - }); - + vm.changeSelection = changeSelection; + vm.dirtyVariantFilter = dirtyVariantFilter; + vm.pristineVariantFilter = pristineVariantFilter; + /** Returns true if publishing is possible based on if there are un-published mandatory languages */ function canPublish() { var selected = []; @@ -57,6 +38,8 @@ function changeSelection(variant) { $scope.model.disableSubmitButton = !canPublish(); + //need to set the Save state to true if publish is true + variant.save = variant.publish; } function dirtyVariantFilter(variant) { @@ -75,6 +58,8 @@ function onInit() { + vm.variants = $scope.model.variants; + if (!$scope.model.title) { localizationService.localize("content_readyToPublish").then(function (value) { $scope.model.title = value; @@ -107,6 +92,7 @@ if (active) { //ensure that the current one is selected active.publish = true; + active.save = true; } $scope.model.disableSubmitButton = !canPublish(); @@ -120,10 +106,13 @@ } + onInit(); + //when this dialog is closed, reset all 'publish' flags $scope.$on('$destroy', function () { for (var i = 0; i < vm.variants.length; i++) { vm.variants[i].publish = false; + vm.variants[i].save = false; } }); } diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.controller.js b/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.controller.js index 7d610e9254..0c138b78a3 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.controller.js @@ -4,31 +4,13 @@ function SaveContentController($scope, localizationService) { var vm = this; - vm.variants = $scope.model.variants; - vm.changeSelection = changeSelection; vm.loading = true; - vm.dirtyVariantFilter = dirtyVariantFilter; - vm.pristineVariantFilter = pristineVariantFilter; vm.hasPristineVariants = false; - //watch this model, if it's reset, then re init - $scope.$watch("model.variants", - function (newVal, oldVal) { - vm.variants = newVal; - if (oldVal && oldVal.length) { - //re-bind the selections - for (var i = 0; i < oldVal.length; i++) { - var found = _.find(vm.variants, function (v) { - return v.language.id === oldVal[i].language.id; - }); - if (found) { - found.save = oldVal[i].save; - } - } - } - onInit(); - }); - + vm.changeSelection = changeSelection; + vm.dirtyVariantFilter = dirtyVariantFilter; + vm.pristineVariantFilter = pristineVariantFilter; + function changeSelection(variant) { var firstSelected = _.find(vm.variants, function (v) { return v.save; @@ -50,6 +32,8 @@ function onInit() { + vm.variants = $scope.model.variants; + if(!$scope.model.title) { localizationService.localize("content_readyToSave").then(function(value){ $scope.model.title = value; @@ -92,6 +76,8 @@ vm.loading = false; } + onInit(); + //when this dialog is closed, reset all 'save' flags $scope.$on('$destroy', function () { for (var i = 0; i < vm.variants.length; i++) { diff --git a/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs b/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs index 51fc80b113..264dd049f5 100644 --- a/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs +++ b/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs @@ -5,6 +5,7 @@ using System.Net; using System.Net.Http; using System.Web.Http.Controllers; using System.Web.Http.ModelBinding; +using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; using Umbraco.Web.Models.ContentEditing; @@ -148,7 +149,16 @@ namespace Umbraco.Web.Editors.Filters // validate var valueEditor = editor.GetValueEditor(p.DataType.Configuration); foreach (var r in valueEditor.Validate(postedValue, p.IsRequired, p.ValidationRegExp)) + { + //this could be a thing, but it does make the errors seem very verbose + ////update the error message to include the property name and culture if available + //r.ErrorMessage = p.Culture.IsNullOrWhiteSpace() + // ? $"'{p.Label}' - {r.ErrorMessage}" + // : $"'{p.Label}' ({p.Culture}) - {r.ErrorMessage}"; + modelState.AddPropertyError(r, p.Alias, p.Culture); + } + } return modelState.IsValid;