From b91a06ab98e3a1764be7cce6776ca782073487ed Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 9 May 2014 16:00:30 +1000 Subject: [PATCH] Fixes: U4-4849 Cannot change a users language/sections in the back office --- .../EntityBase/TracksChangesEntityBase.cs | 10 ++++-- src/Umbraco.Core/Models/Membership/User.cs | 24 ++++++------- .../Persistence/Factories/UserFactory.cs | 2 +- .../Repositories/UserRepository.cs | 18 ++++++---- src/Umbraco.Tests/Models/ContentTests.cs | 8 +++++ src/Umbraco.Tests/Models/ContentTypeTests.cs | 20 +++++++++-- .../Services/UserServiceTests.cs | 35 +++++++++++++++++++ .../umbraco/users/EditUser.aspx.cs | 4 +-- src/umbraco.businesslogic/User.cs | 26 ++++++++++++++ 9 files changed, 119 insertions(+), 28 deletions(-) diff --git a/src/Umbraco.Core/Models/EntityBase/TracksChangesEntityBase.cs b/src/Umbraco.Core/Models/EntityBase/TracksChangesEntityBase.cs index 1d23fe8385..cfc6ce8c9d 100644 --- a/src/Umbraco.Core/Models/EntityBase/TracksChangesEntityBase.cs +++ b/src/Umbraco.Core/Models/EntityBase/TracksChangesEntityBase.cs @@ -17,7 +17,7 @@ namespace Umbraco.Core.Models.EntityBase /// /// Tracks the properties that have changed /// - private readonly IDictionary _propertyChangedInfo = new Dictionary(); + private IDictionary _propertyChangedInfo = new Dictionary(); /// /// Tracks the properties that we're changed before the last commit (or last call to ResetDirtyProperties) @@ -86,7 +86,9 @@ namespace Umbraco.Core.Models.EntityBase /// public void ForgetPreviouslyDirtyProperties() { - _lastPropertyChangedInfo.Clear(); + //NOTE: We cannot .Clear() because when we memberwise clone this will be the SAME + // instance as the one on the clone, so we need to create a new instance. + _lastPropertyChangedInfo = new Dictionary(); } /// @@ -119,7 +121,9 @@ namespace Umbraco.Core.Models.EntityBase _lastPropertyChangedInfo = _propertyChangedInfo.ToDictionary(v => v.Key, v => v.Value); } - _propertyChangedInfo.Clear(); + //NOTE: We cannot .Clear() because when we memberwise clone this will be the SAME + // instance as the one on the clone, so we need to create a new instance. + _propertyChangedInfo = new Dictionary(); } /// diff --git a/src/Umbraco.Core/Models/Membership/User.cs b/src/Umbraco.Core/Models/Membership/User.cs index 88a0df14ef..45bc4aa375 100644 --- a/src/Umbraco.Core/Models/Membership/User.cs +++ b/src/Umbraco.Core/Models/Membership/User.cs @@ -244,7 +244,10 @@ namespace Umbraco.Core.Models.Membership public void RemoveAllowedSection(string sectionAlias) { - _sectionCollection.Remove(sectionAlias); + if (_sectionCollection.Contains(sectionAlias)) + { + _sectionCollection.Remove(sectionAlias); + } } public void AddAllowedSection(string sectionAlias) @@ -425,23 +428,20 @@ namespace Umbraco.Core.Models.Membership { var item = e.NewItems.Cast().First(); - //remove from the removed/added sections (since people could add/remove all they want in one request) - _removedSections.Remove(item); - _addedSections.Remove(item); - - //add to the added sections - _addedSections.Add(item); + if (_addedSections.Contains(item) == false) + { + _addedSections.Add(item); + } } else if (e.Action == NotifyCollectionChangedAction.Remove) { var item = e.OldItems.Cast().First(); - //remove from the removed/added sections (since people could add/remove all they want in one request) - _removedSections.Remove(item); - _addedSections.Remove(item); + if (_removedSections.Contains(item) == false) + { + _removedSections.Add(item); + } - //add to the added sections - _removedSections.Add(item); } } diff --git a/src/Umbraco.Core/Persistence/Factories/UserFactory.cs b/src/Umbraco.Core/Persistence/Factories/UserFactory.cs index 080776e7fe..0f2ce2c5e6 100644 --- a/src/Umbraco.Core/Persistence/Factories/UserFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/UserFactory.cs @@ -74,7 +74,7 @@ namespace Umbraco.Core.Persistence.Factories { AppAlias = app }; - if (entity.Id != null) + if (entity.HasIdentity) { appDto.UserId = (int) entity.Id; } diff --git a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs index a2b91a8447..07714b36a7 100644 --- a/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/UserRepository.cs @@ -211,9 +211,19 @@ namespace Umbraco.Core.Persistence.Repositories var user = (User)entity; if (user.IsPropertyDirty("AllowedSections")) { + //now we need to delete any applications that have been removed + foreach (var section in user.RemovedSections) + { + //we need to manually delete thsi record because it has a composite key + Database.Delete("WHERE app=@Section AND " + SqlSyntaxContext.SqlSyntaxProvider.GetQuotedColumnName("user") + "=@UserId", + new { Section = section, UserId = (int)user.Id }); + } + //for any that exist on the object, we need to determine if we need to update or insert + //NOTE: the User2AppDtos collection wil always be equal to the User.AllowedSections foreach (var sectionDto in userDto.User2AppDtos) { + //if something has been added then insert it if (user.AddedSections.Contains(sectionDto.AppAlias)) { //we need to insert since this was added @@ -227,13 +237,7 @@ namespace Umbraco.Core.Persistence.Repositories } } - //now we need to delete any applications that have been removed - foreach (var section in user.RemovedSections) - { - //we need to manually delete thsi record because it has a composite key - Database.Delete("WHERE app=@Section AND " + SqlSyntaxContext.SqlSyntaxProvider.GetQuotedColumnName("user") + "=@UserId", - new { Section = section, UserId = (int)user.Id }); - } + } ((ICanBeDirty)entity).ResetDirtyProperties(); diff --git a/src/Umbraco.Tests/Models/ContentTests.cs b/src/Umbraco.Tests/Models/ContentTests.cs index 6f9dbc5d46..6ca360c81b 100644 --- a/src/Umbraco.Tests/Models/ContentTests.cs +++ b/src/Umbraco.Tests/Models/ContentTests.cs @@ -224,6 +224,14 @@ namespace Umbraco.Tests.Models { Assert.AreEqual(propertyInfo.GetValue(clone, null), propertyInfo.GetValue(content, null)); } + + //need to ensure the event handlers are wired + + var asDirty = (ICanBeDirty)clone; + + Assert.IsFalse(asDirty.IsPropertyDirty("Properties")); + clone.Properties.Add(new Property(1, Guid.NewGuid(), new PropertyType("test", DataTypeDatabaseType.Ntext) {Alias = "blah"}, "blah")); + Assert.IsTrue(asDirty.IsPropertyDirty("Properties")); } [Test] diff --git a/src/Umbraco.Tests/Models/ContentTypeTests.cs b/src/Umbraco.Tests/Models/ContentTypeTests.cs index 0fc1a52a28..be11337599 100644 --- a/src/Umbraco.Tests/Models/ContentTypeTests.cs +++ b/src/Umbraco.Tests/Models/ContentTypeTests.cs @@ -4,12 +4,13 @@ using NUnit.Framework; using Umbraco.Core.Models; using Umbraco.Core.Models.EntityBase; using Umbraco.Core.Serialization; +using Umbraco.Tests.TestHelpers; using Umbraco.Tests.TestHelpers.Entities; namespace Umbraco.Tests.Models { [TestFixture] - public class ContentTypeTests + public class ContentTypeTests : BaseUmbracoConfigurationTest { [Test] public void Can_Deep_Clone_Content_Type_Sort() @@ -140,19 +141,21 @@ namespace Umbraco.Tests.Models Assert.AreNotSame(clone.AllowedTemplates.ElementAt(index), contentType.AllowedTemplates.ElementAt(index)); Assert.AreEqual(clone.AllowedTemplates.ElementAt(index), contentType.AllowedTemplates.ElementAt(index)); } + Assert.AreNotSame(clone.PropertyGroups, contentType.PropertyGroups); Assert.AreEqual(clone.PropertyGroups.Count, contentType.PropertyGroups.Count); for (var index = 0; index < contentType.PropertyGroups.Count; index++) { Assert.AreNotSame(clone.PropertyGroups[index], contentType.PropertyGroups[index]); Assert.AreEqual(clone.PropertyGroups[index], contentType.PropertyGroups[index]); } + Assert.AreNotSame(clone.PropertyTypes, contentType.PropertyTypes); Assert.AreEqual(clone.PropertyTypes.Count(), contentType.PropertyTypes.Count()); for (var index = 0; index < contentType.PropertyTypes.Count(); index++) { Assert.AreNotSame(clone.PropertyTypes.ElementAt(index), contentType.PropertyTypes.ElementAt(index)); Assert.AreEqual(clone.PropertyTypes.ElementAt(index), contentType.PropertyTypes.ElementAt(index)); } - + Assert.AreEqual(clone.CreateDate, contentType.CreateDate); Assert.AreEqual(clone.CreatorId, contentType.CreatorId); Assert.AreEqual(clone.Key, contentType.Key); @@ -167,13 +170,24 @@ namespace Umbraco.Tests.Models Assert.AreEqual(clone.Thumbnail, contentType.Thumbnail); Assert.AreEqual(clone.Icon, contentType.Icon); Assert.AreEqual(clone.IsContainer, contentType.IsContainer); - + //This double verifies by reflection var allProps = clone.GetType().GetProperties(); foreach (var propertyInfo in allProps) { Assert.AreEqual(propertyInfo.GetValue(clone, null), propertyInfo.GetValue(contentType, null)); } + + //need to ensure the event handlers are wired + + var asDirty = (ICanBeDirty)clone; + + Assert.IsFalse(asDirty.IsPropertyDirty("PropertyTypes")); + clone.AddPropertyType(new PropertyType("test", DataTypeDatabaseType.Nvarchar) { Alias = "blah" }); + Assert.IsTrue(asDirty.IsPropertyDirty("PropertyTypes")); + Assert.IsFalse(asDirty.IsPropertyDirty("PropertyGroups")); + clone.AddPropertyGroup("hello"); + Assert.IsTrue(asDirty.IsPropertyDirty("PropertyGroups")); } [Test] diff --git a/src/Umbraco.Tests/Services/UserServiceTests.cs b/src/Umbraco.Tests/Services/UserServiceTests.cs index f0fdc8a21d..5c41895037 100644 --- a/src/Umbraco.Tests/Services/UserServiceTests.cs +++ b/src/Umbraco.Tests/Services/UserServiceTests.cs @@ -387,6 +387,41 @@ namespace Umbraco.Tests.Services Assert.That(user.DefaultPermissions, Is.EqualTo(userType.Permissions)); } + [Test] + public void Can_Add_And_Remove_Sections_From_User() + { + var userType = ServiceContext.UserService.GetUserTypeByAlias("admin"); + + var user1 = ServiceContext.UserService.CreateUserWithIdentity("test1", "test1@test.com", userType); + + //adds some allowed sections + user1.AddAllowedSection("test1"); + user1.AddAllowedSection("test2"); + user1.AddAllowedSection("test3"); + user1.AddAllowedSection("test4"); + ServiceContext.UserService.Save(user1); + + var result1 = ServiceContext.UserService.GetUserById((int)user1.Id); + Assert.AreEqual(4, result1.AllowedSections.Count()); + + //simulate clearing the sections + foreach (var s in user1.AllowedSections) + { + result1.RemoveAllowedSection(s); + } + //now just re-add a couple + result1.AddAllowedSection("test3"); + result1.AddAllowedSection("test4"); + ServiceContext.UserService.Save(result1); + + //assert + + //re-get + result1 = ServiceContext.UserService.GetUserById((int)user1.Id); + Assert.AreEqual(2, result1.AllowedSections.Count()); + + } + [Test] public void Can_Remove_Section_From_All_Assigned_Users() { diff --git a/src/Umbraco.Web/umbraco.presentation/umbraco/users/EditUser.aspx.cs b/src/Umbraco.Web/umbraco.presentation/umbraco/users/EditUser.aspx.cs index 1f0262a1d0..b1d58bccea 100644 --- a/src/Umbraco.Web/umbraco.presentation/umbraco/users/EditUser.aspx.cs +++ b/src/Umbraco.Web/umbraco.presentation/umbraco/users/EditUser.aspx.cs @@ -489,10 +489,10 @@ namespace umbraco.cms.presentation.user } u.StartMediaId = mstartNode; - u.clearApplications(); + u.ClearApplications(); foreach (ListItem li in lapps.Items) { - if (li.Selected) u.addApplication(li.Value); + if (li.Selected) u.AddApplication(li.Value); } u.Save(); diff --git a/src/umbraco.businesslogic/User.cs b/src/umbraco.businesslogic/User.cs index ef1b736d73..3a135d1834 100644 --- a/src/umbraco.businesslogic/User.cs +++ b/src/umbraco.businesslogic/User.cs @@ -689,9 +689,22 @@ namespace umbraco.BusinessLogic get { return _user.Id; } } + /// + /// Clears the list of applications the user has access to, ensure to call Save afterwords + /// + public void ClearApplications() + { + if (_lazyId.HasValue) SetupUser(_lazyId.Value); + foreach (var s in _user.AllowedSections.ToArray()) + { + _user.RemoveAllowedSection(s); + } + } + /// /// Clears the list of applications the user has access to. /// + [Obsolete("This method will implicitly cause a database save and will reset the current user's dirty property, do not use this method, use the ClearApplications method instead and then call Save() when you are done performing all user changes to persist the chagnes in one transaction")] public void clearApplications() { if (_lazyId.HasValue) SetupUser(_lazyId.Value); @@ -701,19 +714,32 @@ namespace umbraco.BusinessLogic _user.RemoveAllowedSection(s); } + //For backwards compatibility this requires an implicit save ApplicationContext.Current.Services.UserService.Save(_user); } + /// + /// Adds a application to the list of allowed applications, ensure to call Save() afterwords + /// + /// + public void AddApplication(string appAlias) + { + if (_lazyId.HasValue) SetupUser(_lazyId.Value); + _user.AddAllowedSection(appAlias); + } + /// /// Adds a application to the list of allowed applications /// /// The app alias. + [Obsolete("This method will implicitly cause a multiple database saves and will reset the current user's dirty property, do not use this method, use the AddApplication method instead and then call Save() when you are done performing all user changes to persist the chagnes in one transaction")] public void addApplication(string AppAlias) { if (_lazyId.HasValue) SetupUser(_lazyId.Value); _user.AddAllowedSection(AppAlias); + //For backwards compatibility this requires an implicit save ApplicationContext.Current.Services.UserService.Save(_user); }