Add store owner #81

RayMMond · 2020-08-03T01:17:14Z

This PR resolve #68 and :

upgrade abp to 3.0.5.
add store owner and store owner permission check.
some minor fixes.

add product tag plugin and store approval plugin. add store owner and store owner permission check.

gdlcf88

add product tag plugin and open store approval plugin.

Hi @RayMMond, I did not see any new plugin in this PR. I noticed that the code relate to the ProductTag is added to the Products module directly, what if I do not install the ProductTag plugin?

...p.Orders/src/EasyAbp.EShop.Orders.Application/EasyAbp/EShop/Orders/Orders/OrderAppService.cs

gdlcf88 · 2020-08-03T08:52:30Z

...p.Orders/src/EasyAbp.EShop.Orders.Application/EasyAbp/EShop/Orders/Orders/OrderAppService.cs

-                await AuthorizationService.CheckAsync(OrdersPermissions.Orders.Manage);
-
-                // Todo: Check if current user is an admin of the store.
+                await AuthorizationService.CheckStoreOwnerAsync(order.StoreId, OrdersPermissions.Orders.Manage);


A store side AuthorizationRequirement may be better, we can define a StoreOwnerAuthorizationRequirement to check if the current user is a owner of the store (it does not care about the permission requirement). Please refer to the practice of https://github.com/EasyAbp/FileManagement/blob/master/src/EasyAbp.FileManagement.Application/EasyAbp/FileManagement/Files/FileOperationAuthorizationHandler.cs

I see an AuthorizationHandler below, but the idea of this comment still has reference significance. (keep the PR lightweight since a huge PR is really hard to review)

gdlcf88 · 2020-08-03T08:57:35Z

.../src/EasyAbp.EShop.Payments.Application/EasyAbp/EShop/Payments/Payments/PaymentAppService.cs

+                }
+                else
+                {
+                    await AuthorizationService.CheckAsync(PaymentsPermissions.Payments.Manage);


Should also have PaymentsPermissions.Payments.CrossStore permission. I think there may be many similar problems, I did not check all the code in this PR, please check it, thank you.

gdlcf88 · 2020-08-03T09:17:30Z

...ation.Contracts/EasyAbp/EShop/Stores/Permissions/BasicStorePermissionAuthorizationHandler.cs

+            var userId = context.User?.FindUserId();
+            if (userId.HasValue)
+            {
+                var isStoreOwner = await _storeOwnerAppService.IsStoreOwnerAsync(requirement.StoreId, userId.Value);


Should use IStoreOwenerStore.IsOwnerAsync()

gdlcf88 · 2020-08-03T09:29:35Z

...rc/EasyAbp.EShop.Stores.Application/EasyAbp/EShop/Stores/StoreOwners/StoreOwnerAppService.cs

+    public class StoreOwnerAppService : ReadOnlyAppService<StoreOwner, StoreOwnerDto, Guid, GetStoreOwnerListDto>,
+        IStoreOwnerAppService
+    {
+        protected override string GetListPolicyName { get; set; } = StoresPermissions.Stores.Default;


What about the "get" policy?

Should people who have StoresPermissions.Stores.Default be allowed to get other stores' and users' StoreOwner?

gdlcf88 · 2020-08-03T09:34:20Z

...p.Stores/src/EasyAbp.EShop.Stores.Application/EasyAbp/EShop/Stores/Stores/StoreAppService.cs

+
+            MapToEntity(input, entity);
+
+            entity = await _storeManager.UpdateAsync(entity, input.OwnerIds);


The best practice of domain services is only creating/updating data of entities and use the repositories to persist the data in application services.

gdlcf88 · 2020-08-03T09:38:56Z

...Shop.Stores/src/EasyAbp.EShop.Stores.Web/Pages/EShop/Stores/Stores/Store/EditModal.cshtml.cs

+            StoreOwners =
+                (await _userAppService.GetListAsync(new GetIdentityUsersInput
+                { MaxResultCount = LimitedResultRequestDto.MaxMaxResultCount })).Items
+                .Select(x => new SelectListItem(x.UserName, x.Id.ToString())).ToList();
+


What if there are more than 1000 users?

@gdlcf88 I just follow the implementation of product categories, see this:

EShop/modules/EasyAbp.EShop.Products/src/EasyAbp.EShop.Products.Web/Pages/EShop/Products/Products/Product/EditModal.cshtml.cs

Lines 56 to 64 in ac750c4

ProductTypes =

(await _productTypeAppService.GetListAsync(new PagedAndSortedResultRequestDto

{MaxResultCount = LimitedResultRequestDto.MaxMaxResultCount})).Items

.Select(dto => new SelectListItem(dto.DisplayName, dto.Id.ToString())).ToList();

Categories =

(await _categoryAppService.GetListAsync(new GetCategoryListDto

{MaxResultCount = LimitedResultRequestDto.MaxMaxResultCount}))?.Items

.Select(dto => new SelectListItem(dto.DisplayName, dto.Id.ToString())).ToList();

The count of ProductType and Category is expected to be less than 1000, but the count of User is easy to be more than 1000.

I think the best practice should be selecting from a modal with filtered user list, but I have no clue how to implement filtered list in abp, do you have any examples?

Maybe using UserId directly before we have a better way.

gdlcf88 · 2020-08-03T09:43:43Z

...Abp.EShop.Stores/src/EasyAbp.EShop.Stores.Domain/EasyAbp/EShop/Stores/Stores/StoreManager.cs

+            return store;
+        }
+
+        protected virtual async Task UpdateStoreOwnersAsync(Guid storeId, IEnumerable<Guid> ownerIds)


It will be easier to manage if the StoreOwner has its own application service and management pages (remember it is an aggregate root). Then the StoreManager can be removed.

…StoreOwner

…vice injections

gdlcf88 · 2020-08-21T11:34:16Z

...op.Stores.Application/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyReadOnlyAppService.cs

+        protected virtual async Task CheckMultiStorePolicyAsync(Guid storeId, string policyName)
+        {
+            if (!await AuthorizationService.IsStoreOwnerGrantedAsync(storeId, policyName))
+            {
+                await CheckPolicyAsync(CrossStorePolicyName);
+                await CheckPolicyAsync(policyName);
+            }
+        }


The storeId can be nullable and check the cross store permission if it is null.

gdlcf88 · 2020-08-21T11:36:40Z

...p.Orders/src/EasyAbp.EShop.Orders.Application/EasyAbp/EShop/Orders/Orders/OrderAppService.cs

                if (input.StoreId.HasValue)
                {
-                    // Todo: Check if current user is an admin of the store.
+                    await CheckMultiStorePolicyAsync(input.StoreId.Value, GetListPolicyName);
                }
                else
                {
-                    await AuthorizationService.CheckAsync(OrdersPermissions.Orders.CrossStore);
+                    throw new AbpAuthorizationException("Authorization failed! Given policy has not granted: " +
+                                                        GetListPolicyName);
                }


We can use input.StoreId directly as the param of CheckMultiStorePolicyAsync() method.

...p.Orders/src/EasyAbp.EShop.Orders.Application/EasyAbp/EShop/Orders/Orders/OrderAppService.cs

gdlcf88 · 2020-08-21T11:54:15Z

...ation.Contracts/EasyAbp/EShop/Stores/Permissions/BasicStorePermissionAuthorizationHandler.cs

+            {
+                var isStoreOwner = await _storeOwnerStore.IsStoreOwnerAsync(requirement.StoreId, userId.Value);
+
+                if (isStoreOwner)


What if the current user is a cross store manager?

Every domain can have their own cross-store permission definition, so it should be implemented separately. And I have created base classes which contains store-owner permission and cross-store permission check, here:https://github.com/RayMMond/EShop/blob/29080d62532c90d330077e50b85ca7d7031fc174/modules/EasyAbp.EShop.Stores/src/EasyAbp.EShop.Stores.Application/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyCrudAppService.cs

gdlcf88 · 2020-08-21T12:19:15Z

...ation.Contracts/EasyAbp/EShop/Stores/Permissions/BasicStorePermissionAuthorizationHandler.cs

+                        context.Succeed(requirement);
+                    }
+                }
+            }


Maybe set failed result here?

A requirement can have multiple AuthorizationHandler, and module user may register their own Handers, so I think we should not set failed here.

gdlcf88 · 2020-08-21T12:20:15Z

....Application.Contracts/EasyAbp/EShop/Stores/Permissions/StoreOwnerAuthorizationExtensions.cs

+{
+    public static class StoreOwnerAuthorizationExtensions
+    {
+        public static Task<bool> IsStoreOwnerGrantedAsync(this IAuthorizationService authorizationService,


What is store owner? Does it include cross store managers?

gdlcf88 · 2020-08-21T12:27:02Z

...eworkCore/EasyAbp/EShop/Stores/EntityFrameworkCore/StoresDbContextModelCreatingExtensions.cs

+                b.ConfigureByConvention();
+                /* Configure more properties here */
+
+                b.HasIndex(x => new {x.StoreId, x.OwnerId})


Should be new {x.OwnerId, x.StoreId}.

And it is better to change all the usage of x.StoreId == storeId && x.OwnerId == ownerId to x.OwnerId == ownerId && x.StoreId == storeId.

gdlcf88 · 2020-08-21T12:27:59Z

modules/EasyAbp.EShop.Stores/src/EasyAbp.EShop.Stores.Web/EasyAbp.EShop.Stores.Web.csproj

+      <DependentUpon>Index.cshtml</DependentUpon>
+    </Compile>
+  </ItemGroup>
+


Unused items in ItemGroup should be cleaned.

gdlcf88 · 2020-08-21T12:30:25Z

....EShop.Stores/src/EasyAbp.EShop.Stores.Domain/EasyAbp/EShop/Stores/StoreOwners/StoreOwner.cs

+
+        public virtual Guid StoreId { get; protected set; }
+
+        public virtual Guid OwnerId { get; protected set; }


OwnerUserId is a more accurate name.

gdlcf88 · 2020-08-21T12:43:16Z

...bp.EShop.Orders/src/EasyAbp.EShop.Orders.Application/EasyAbp.EShop.Orders.Application.csproj

    <ProjectReference Include="..\..\..\EasyAbp.EShop.Products\src\EasyAbp.EShop.Products.Application.Contracts\EasyAbp.EShop.Products.Application.Contracts.csproj" />
+    <ProjectReference Include="..\..\..\EasyAbp.EShop.Stores\src\EasyAbp.EShop.Stores.Application\EasyAbp.EShop.Stores.Application.csproj" />


We should not do that since the entities in Stores module can be distributed.

…ication.Shared 2. minor refactoring

gdlcf88

We should remove the related cache after a user's store ownership is changed.

...res.Application.Shared/EasyAbp/EShop/Stores/Permissions/StoreOwnerAuthorizationExtensions.cs

...Stores.Application.Shared/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyCrudAppService.cs

...res.Application.Shared/EasyAbp/EShop/Stores/Permissions/StoreOwnerAuthorizationExtensions.cs

...Products.Application/EasyAbp/EShop/Products/ProductInventories/ProductInventoryAppService.cs

...es.Application.Shared/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyReadOnlyAppService.cs

gdlcf88 · 2020-08-27T18:42:07Z

...rc/EasyAbp.EShop.Stores.Application/EasyAbp/EShop/Stores/StoreOwners/StoreOwnerAppService.cs

+        {
+            _repository = repository;
+        }
+


You are right, please remove the IStoreOwnerStore injection.

remove the related cache after a user's store ownership is changed.

...res.Application.Shared/EasyAbp/EShop/Stores/Permissions/StoreOwnerAuthorizationExtensions.cs

...Stores.Application.Shared/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyCrudAppService.cs

...es.Application.Shared/EasyAbp/EShop/Stores/Stores/MultiStoreAbstractKeyReadOnlyAppService.cs

...Products.Application/EasyAbp/EShop/Products/ProductInventories/ProductInventoryAppService.cs

gdlcf88 · 2020-08-31T12:56:39Z

.../src/EasyAbp.EShop.Products.Application/EasyAbp/EShop/Products/Products/ProductAppService.cs

 using Volo.Abp;
 using Volo.Abp.Application.Dtos;
 using Volo.Abp.Application.Services;
 using Volo.Abp.Domain.Entities;

 namespace EasyAbp.EShop.Products.Products
 {
-    public class ProductAppService : CrudAppService<Product, ProductDto, Guid, GetProductListDto, CreateUpdateProductDto, CreateUpdateProductDto>,
+    public class ProductAppService : CrudAppService<Product, ProductDto, Guid, GetProductListDto, CreateUpdateProductDto


Can ProductAppService inherit MultiStoreCrudAppService?

Product don't inherit IMultiStore, so answer is no.

…ase method

gdlcf88 · 2020-09-01T09:19:53Z

Thank you @RayMMond for your great work.

update abp to 3.0.5.

467f317

add product tag plugin and store approval plugin. add store owner and store owner permission check.

gdlcf88 reviewed Aug 3, 2020

View reviewed changes

Remove plugins

5ffbe8c

RayMMond changed the title ~~Add store owner and two plugins~~ Add store owner Aug 4, 2020

RayMMond added 6 commits August 5, 2020 20:07

remove unnecessary changes

b3369d0

Merge remote-tracking branch 'remotes/EasyAbp/dev' into ProductTagAnd…

4ca82e3

…StoreOwner

make StoreOwner has its own application service and management pages

530f82e

Merge remote-tracking branch 'remotes/EasyAbp/dev' into ProductTagAnd…

506b18e

…StoreOwner

Add multi store crud app service base classes, remove unnecessary ser…

16d99b2

…vice injections

StoreDto use right extensible base class, revert unwanted changes

80493f4

RayMMond requested a review from gdlcf88 August 15, 2020 09:52

gdlcf88 reviewed Aug 21, 2020

View reviewed changes

gdlcf88 mentioned this pull request Aug 21, 2020

Add product tag plugin #87

Draft

RayMMond added 3 commits August 26, 2020 14:44

Merge branch 'dev' into ProductTagAndStoreOwner

29080d6

1. Move store owner related base classes to EasyAbp.EShop.Stores.Appl…

0335be9

…ication.Shared 2. minor refactoring

fix code format

22e4249

RayMMond requested a review from gdlcf88 August 26, 2020 09:34

RayMMond added 2 commits August 26, 2020 22:19

Merge branch 'dev' into ProductTagAndStoreOwner

4269c54

add missing semicolon

968eee8

gdlcf88 reviewed Aug 27, 2020

View reviewed changes

RayMMond added 2 commits August 31, 2020 17:57

fix store owner extension methods.

54036e1

remove the related cache after a user's store ownership is changed.

Merge branch 'dev' into ProductTagAndStoreOwner

d6dd179

RayMMond requested a review from gdlcf88 August 31, 2020 10:01

gdlcf88 reviewed Aug 31, 2020

View reviewed changes

RayMMond and others added 5 commits September 1, 2020 10:39

fix store related extension method naming, add crossStoreAllowed to b…

fc21c49

…ase method

Merge branch 'dev' into ProductTagAndStoreOwner

b47a34e

fix samples build error.

7c786f8

Complete changes of store owner

9efc63f

Fix the routes in ProductController

609b1cd

gdlcf88 merged commit b782323 into EasyAbp:dev Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add store owner #81

Add store owner #81

RayMMond commented Aug 3, 2020 •

edited

Loading

gdlcf88 left a comment

gdlcf88 Aug 3, 2020

gdlcf88 Aug 3, 2020

gdlcf88 Aug 3, 2020

RayMMond Aug 3, 2020

gdlcf88 Aug 3, 2020

gdlcf88 Aug 3, 2020

gdlcf88 Aug 3, 2020

gdlcf88 Aug 3, 2020

RayMMond Aug 3, 2020 •

edited

Loading

gdlcf88 Aug 11, 2020

RayMMond Aug 12, 2020

gdlcf88 Aug 12, 2020

RayMMond Aug 12, 2020

gdlcf88 Aug 3, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

RayMMond Aug 26, 2020

gdlcf88 Aug 21, 2020

RayMMond Aug 26, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

gdlcf88 Aug 21, 2020

gdlcf88 left a comment

gdlcf88 Aug 27, 2020

gdlcf88 Aug 31, 2020

RayMMond Sep 1, 2020

gdlcf88 commented Sep 1, 2020


		MapToEntity(input, entity);

		entity = await _storeManager.UpdateAsync(entity, input.OwnerIds);

	ProductTypes =
	(await _productTypeAppService.GetListAsync(new PagedAndSortedResultRequestDto
	{MaxResultCount = LimitedResultRequestDto.MaxMaxResultCount})).Items
	.Select(dto => new SelectListItem(dto.DisplayName, dto.Id.ToString())).ToList();

	Categories =
	(await _categoryAppService.GetListAsync(new GetCategoryListDto
	{MaxResultCount = LimitedResultRequestDto.MaxMaxResultCount}))?.Items
	.Select(dto => new SelectListItem(dto.DisplayName, dto.Id.ToString())).ToList();


		public virtual Guid StoreId { get; protected set; }

		public virtual Guid OwnerId { get; protected set; }

		<ProjectReference Include="..\..\..\EasyAbp.EShop.Products\src\EasyAbp.EShop.Products.Application.Contracts\EasyAbp.EShop.Products.Application.Contracts.csproj" />
		<ProjectReference Include="..\..\..\EasyAbp.EShop.Stores\src\EasyAbp.EShop.Stores.Application\EasyAbp.EShop.Stores.Application.csproj" />

Add store owner #81

Add store owner #81

Conversation

RayMMond commented Aug 3, 2020 • edited Loading

gdlcf88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RayMMond Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdlcf88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdlcf88 commented Sep 1, 2020

RayMMond commented Aug 3, 2020 •

edited

Loading

RayMMond Aug 3, 2020 •

edited

Loading