Changed controller to use role manager. Updated mapping. Don't commit test database.

.gitignore

@@ -202,3 +202,4 @@ src/Umbraco.Tests/TEMP/
 /src/Umbraco.Web.UI/config/umbracoSettings.config
 /src/Umbraco.Web.UI.NetCore/Umbraco/models/*
 src/Umbraco.Tests.UnitTests/umbraco/Data/TEMP/
+src/Umbraco.Tests.Integration/DatabaseContextTests.sdf

src/Umbraco.Infrastructure/Security/MemberRoleStore.cs

@@ -246,12 +246,25 @@ namespace Umbraco.Cms.Core.Security
                 throw new ArgumentNullException(nameof(roleId));
             }
 
+            IMemberGroup memberGroup;
+
+            // member group can be found by int or Guid, so try both
             if (!int.TryParse(roleId, out int id))
             {
-                throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Int");
+                if (!Guid.TryParse(roleId, out Guid guid))
+                {
+                    throw new ArgumentOutOfRangeException(nameof(roleId), $"{nameof(roleId)} is not a valid Guid");
+                }
+                else
+                {
+                    memberGroup = _memberGroupService.GetById(guid);
+                }
+            }
+            else
+            {
+                memberGroup = _memberGroupService.GetById(id);
             }
 
-            IMemberGroup memberGroup = _memberGroupService.GetById(id);
             return Task.FromResult(memberGroup == null ? null : MapFromMemberGroup(memberGroup));
         }
 

src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberRoleStoreTests.cs

@@ -251,7 +251,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security
         }
 
         [Test]
-        public async Task GivenIFindAMemberRoleByRoleId_AndRoleIdExists_ThenIShouldGetASuccessResultAsync()
+        public async Task GivenIFindAMemberRoleByRoleKey_AndRoleKeyExists_ThenIShouldGetASuccessResultAsync()
         {
             // arrange
             MemberRoleStore<IdentityRole> sut = CreateSut();
@@ -265,7 +265,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security
             {
                 Name = "fakeGroupName",
                 CreatorId = 123,
-                Id = 777
+                Id = 777,
+                Key = Guid.NewGuid()
             };
 
             _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup);
@@ -281,7 +282,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security
         }
 
         [Test]
-        public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnInt_ThenIShouldGetAFailureResultAsync()
+        public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntOrGuid_ThenIShouldGetAFailureResultAsync()
         {
             // arrange
             MemberRoleStore<IdentityRole> sut = CreateSut();
@@ -300,6 +301,72 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security
             _mockMemberGroupService.VerifyNoOtherCalls();
         }
 
+        [Test]
+        public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAnIntButCanBeToGuid_ThenIShouldGetASuccessResultAsync()
+        {
+            // arrange
+            MemberRoleStore<IdentityRole> sut = CreateSut();
+            var fakeRole = new IdentityRole("fakeGroupName")
+            {
+                Id = "777"
+            };
+
+            var fakeRoleGuid = Guid.NewGuid();
+
+            IMemberGroup fakeMemberGroup = new MemberGroup()
+            {
+                Name = "fakeGroupName",
+                CreatorId = 123,
+                Id = 777,
+                Key = fakeRoleGuid
+            };
+
+            _mockMemberGroupService.Setup(x => x.GetById(fakeRoleGuid)).Returns(fakeMemberGroup);
+
+            // act
+            IdentityRole actual = await sut.FindByIdAsync(fakeRoleGuid.ToString());
+
+            // assert
+            Assert.AreEqual(fakeRole.Name, actual.Name);
+            Assert.AreEqual(fakeRole.Id, actual.Id);
+            _mockMemberGroupService.Verify(x => x.GetById(fakeRoleGuid));
+            _mockMemberGroupService.VerifyNoOtherCalls();
+        }
+
+
+        [Test]
+        public async Task GivenIFindAMemberRoleByRoleId_AndIdCannotBeParsedToAGuidButCanBeToInt_ThenIShouldGetASuccessResultAsync()
+        {
+            // arrange
+            MemberRoleStore<IdentityRole> sut = CreateSut();
+            var fakeRole = new IdentityRole("fakeGroupName")
+            {
+                Id = "777"
+            };
+
+            var fakeRoleId = 777;
+
+            IMemberGroup fakeMemberGroup = new MemberGroup()
+            {
+                Name = "fakeGroupName",
+                CreatorId = 123,
+                Id = 777,
+                Key = Guid.NewGuid()
+            };
+
+            _mockMemberGroupService.Setup(x => x.GetById(fakeRoleId)).Returns(fakeMemberGroup);
+
+            // act
+            IdentityRole actual = await sut.FindByIdAsync(fakeRoleId.ToString());
+
+            // assert
+            Assert.AreEqual(fakeRole.Name, actual.Name);
+            Assert.AreEqual(fakeRole.Id, actual.Id);
+            _mockMemberGroupService.Verify(x => x.GetById(fakeRoleId));
+            _mockMemberGroupService.VerifyNoOtherCalls();
+        }
+
+
         [Test]
         public async Task GivenIFindAMemberRoleByRoleName_AndRoleNameExists_ThenIShouldGetASuccessResultAsync()
         {

src/Umbraco.Web.BackOffice/Controllers/MemberController.cs

@@ -190,6 +190,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
         [OutgoingEditorModelEvent]
         public MemberDisplay GetByKey(Guid key)
         {
+            //TODO: convert to identity
             IMember foundMember = _memberService.GetByKey(key);
             if (foundMember == null)
             {

src/Umbraco.Web.BackOffice/Controllers/MemberGroupController.cs

@@ -48,15 +48,23 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
         /// </summary>
         /// <param name="id"></param>
         /// <returns></returns>
-        public ActionResult<MemberGroupDisplay> GetById(int id)
+        public async Task<ActionResult<MemberGroupDisplay>> GetById(int id)
         {
-            IdentityRole<string> memberGroup = _roleManager.FindByIdAsync(id.ToString()).Result;
+            //TODO: did we envisage this - combination of service and identity manager?
+            IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString());
+            if (identityRole == null)
+            {
+                return NotFound();
+            }
+
+            IMemberGroup memberGroup = _memberGroupService.GetById(id);
             if (memberGroup == null)
             {
                 return NotFound();
             }
 
-            MemberGroupDisplay dto = _umbracoMapper.Map<IdentityRole<string>, MemberGroupDisplay>(memberGroup);
+            //TODO: the default identity role doesn't have all the properties IMemberGroup had, e.g. CreatorId
+            MemberGroupDisplay dto = _umbracoMapper.Map<IMemberGroup, MemberGroupDisplay>(memberGroup);
             return dto;
         }
 
@@ -66,15 +74,21 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
         /// </summary>
         /// <param name="id"></param>
         /// <returns></returns>
-        public ActionResult<MemberGroupDisplay> GetById(Guid id)
+        public async Task<ActionResult<MemberGroupDisplay>> GetById(Guid id)
         {
-            IMemberGroup memberGroup = _memberGroupService.GetById(id);
+            //TODO: did we envisage just identity or a combination of service and identity manager?
-            if (memberGroup == null)
+            IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString());
+            if (identityRole == null)
             {
                 return NotFound();
             }
+            //IMemberGroup memberGroup = _memberGroupService.GetById(id);
+            //if (memberGroup == null)
+            //{
+            //    return NotFound();
+            //}
 
-            return _umbracoMapper.Map<IMemberGroup, MemberGroupDisplay>(memberGroup);
+            return _umbracoMapper.Map<IdentityRole, MemberGroupDisplay>(identityRole);
         }
 
         /// <summary>
@@ -82,7 +96,7 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
         /// </summary>
         /// <param name="id"></param>
         /// <returns></returns>
-        public ActionResult<MemberGroupDisplay> GetById(Udi id)
+        public async Task<ActionResult<MemberGroupDisplay>> GetById(Udi id)
         {
             var guidUdi = id as GuidUdi;
             if (guidUdi == null)
@@ -90,43 +104,53 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
                 return NotFound();
             }
 
-            IMemberGroup memberGroup = _memberGroupService.GetById(guidUdi.Guid);
+            //TODO: can we do this via identity?
-            if (memberGroup == null)
+            IdentityRole identityRole = await _roleManager.FindByIdAsync(id.ToString());
+            if (identityRole == null)
             {
                 return NotFound();
             }
 
-            return _umbracoMapper.Map<IMemberGroup, MemberGroupDisplay>(memberGroup);
+            return _umbracoMapper.Map<IdentityRole, MemberGroupDisplay>(identityRole);
         }
 
-        public IEnumerable<MemberGroupDisplay> GetByIds([FromQuery]int[] ids)
+        public async Task<IEnumerable<MemberGroupDisplay>> GetByIds([FromQuery]int[] ids)
         {
-            var roles = new List<IdentityRole<string>>();
+            var roles = new List<IdentityRole>();
 
             foreach (int id in ids)
             {
-                Task<IdentityRole> role = _roleManager.FindByIdAsync(id.ToString());
+                IdentityRole role = await _roleManager.FindByIdAsync(id.ToString());
-                roles.Add(role.Result);
+                roles.Add(role);
             }
 
-            return roles.Select(x=> _umbracoMapper.Map<IdentityRole<string>, MemberGroupDisplay>(x));
+            return roles.Select(x=> _umbracoMapper.Map<IdentityRole, MemberGroupDisplay>(x));
         }
 
         [HttpDelete]
         [HttpPost]
-        public IActionResult DeleteById(int id)
+        public async Task<IActionResult> DeleteById(int id)
         {
-            IMemberGroup memberGroup = _memberGroupService.GetById(id);
+            //TODO: are there any repercussions elsewhere for us changing these to async?
-            if (memberGroup == null)
+            IdentityRole role = await _roleManager.FindByIdAsync(id.ToString());
+
+            if (role == null)
             {
                 return NotFound();
             }
 
-            _memberGroupService.Delete(memberGroup);
+            IdentityResult roleDeleted = await _roleManager.DeleteAsync(role);
-            return Ok();
+            if (roleDeleted.Succeeded)
+            {
+                return Ok();
+            }
+            else
+            {
+                return Problem("Issue during deletion - please see logs");
+            }
         }
 
-        public IEnumerable<MemberGroupDisplay> GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map<IdentityRole<string>, MemberGroupDisplay>(x));
+        public IEnumerable<MemberGroupDisplay> GetAllGroups() => _roleManager.Roles.Select(x => _umbracoMapper.Map<IdentityRole, MemberGroupDisplay>(x));
 
         public MemberGroupDisplay GetEmpty()
         {
@@ -134,20 +158,28 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers
             return _umbracoMapper.Map<IMemberGroup, MemberGroupDisplay>(item);
         }
 
-        public ActionResult<MemberGroupDisplay> PostSave(MemberGroupSave saveModel)
+        public async Task<ActionResult<MemberGroupDisplay>> PostSave(MemberGroupSave saveModel)
         {
+            int id = int.Parse(saveModel.Id.ToString());
 
-            var id = int.Parse(saveModel.Id.ToString());
+            IdentityRole role = id > 0 ? await _roleManager.FindByIdAsync(saveModel.Id.ToString()) : null;
-            var memberGroup = id > 0 ? _memberGroupService.GetById(id) : new MemberGroup();
+
-            if (memberGroup == null)
+            if (role == null)
             {
                 return NotFound();
             }
 
-            memberGroup.Name = saveModel.Name;
+            role.Name = saveModel.Name;
-            _memberGroupService.Save(memberGroup);
+            IdentityResult updatedResult = await _roleManager.UpdateAsync(role);
 
-            var display = _umbracoMapper.Map<IMemberGroup, MemberGroupDisplay>(memberGroup);
+            if (!updatedResult.Succeeded)
+            {
+                //TODO: what to retrun if there is a failed identity result
+                return Problem();
+            }
+
+            //TODO: should we return the identity role or return the group from the service?
+            MemberGroupDisplay display = _umbracoMapper.Map<IdentityRole, MemberGroupDisplay>(role);
 
             display.AddSuccessNotification(
                             _localizedTextService.Localize("speechBubbles/memberGroupSavedHeader"),

src/Umbraco.Web.BackOffice/Mapping/MemberMapDefinition.cs

@@ -1,4 +1,6 @@
+using System;
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Identity;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Mapping;
 using Umbraco.Cms.Core.Models;
@@ -33,6 +35,7 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping
             mapper.Define<IMember, MemberDisplay>((source, context) => new MemberDisplay(), Map);
             mapper.Define<IMember, MemberBasic>((source, context) => new MemberBasic(), Map);
             mapper.Define<IMemberGroup, MemberGroupDisplay>((source, context) => new MemberGroupDisplay(), Map);
+            mapper.Define<IdentityRole, MemberGroupDisplay>((source, context) => new MemberGroupDisplay(), Map);
             mapper.Define<IMember, ContentPropertyCollectionDto>((source, context) => new ContentPropertyCollectionDto(), Map);
         }
 
@@ -100,5 +103,18 @@ namespace Umbraco.Cms.Web.BackOffice.Mapping
         {
             target.Properties = context.MapEnumerable<IProperty, ContentPropertyDto>(source.Properties);
         }
+
+        /// <summary>
+        /// Maps an identity role to a member group display
+        /// </summary>
+        /// <param name="arg1"></param>
+        /// <param name="arg2"></param>
+        /// <param name="arg3"></param>
+        private void Map(IdentityRole source, MemberGroupDisplay target, MapperContext context)
+        {
+            //TODO: this is all that is mapped at this time, we're losing a lot of properties
+            target.Id = source.Id;
+            target.Name = source.Name;
+        }
     }
 }