From ada775e0744ec494bb76d01ef57c8586f6200e9c Mon Sep 17 00:00:00 2001
From: sauls8t <saul@documize.com>
Date: Tue, 6 Mar 2018 15:40:00 +0000
Subject: [PATCH] Ensure permission records parsed for groups and users

---
 domain/permission/endpoint.go   |  3 +-
 domain/permission/permission.go | 68 ++++++++++++++++++++++++---------
 2 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/domain/permission/endpoint.go b/domain/permission/endpoint.go
index e6697a4c..ac4a6685 100644
--- a/domain/permission/endpoint.go
+++ b/domain/permission/endpoint.go
@@ -656,11 +656,10 @@ func (h *Handler) SetDocumentPermissions(w http.ResponseWriter, r *http.Request)
 		perm.OrgID = ctx.OrgID
 		perm.DocumentID = id
 
+		// get group records for just this group
 		isGroup := perm.Who == permission.GroupPermission
 		groupRecords := []group.Record{}
-
 		if isGroup {
-			// get group records for just this group
 			groupRecords = group.FilterGroupRecords(groupMembers, perm.WhoID)
 		}
 
diff --git a/domain/permission/permission.go b/domain/permission/permission.go
index 17cc86f1..a7b120fc 100644
--- a/domain/permission/permission.go
+++ b/domain/permission/permission.go
@@ -15,6 +15,7 @@ import (
 	"database/sql"
 
 	"github.com/documize/community/domain"
+	group "github.com/documize/community/model/group"
 	pm "github.com/documize/community/model/permission"
 	u "github.com/documize/community/model/user"
 )
@@ -193,34 +194,63 @@ func GetDocumentApprovers(ctx domain.RequestContext, s domain.Store, spaceID, do
 	users = []u.User{}
 	prev := make(map[string]bool) // used to ensure we only process user once
 
-	// check space permissions
-	sp, err := s.Permission.GetSpacePermissions(ctx, spaceID)
-	for _, p := range sp {
-		if p.Action == pm.DocumentApprove {
-			user, err := s.User.Get(ctx, p.WhoID)
-			if err == nil {
-				prev[user.RefID] = true
-				users = append(users, user)
-			} else {
-				return users, err
-			}
-		}
+	// Permissions can be assigned to both groups and individual users.
+	// Pre-fetch users with group membership to help us work out
+	// if user belongs to a group with permissions.
+	groupMembers, err := s.Group.GetMembers(ctx)
+	if err != nil {
+		return users, err
 	}
 
-	// check document permissions
+	// space permissions
+	sp, err := s.Permission.GetSpacePermissions(ctx, spaceID)
+	if err != nil {
+		return users, err
+	}
+	// document permissions
 	dp, err := s.Permission.GetDocumentPermissions(ctx, documentID)
-	for _, p := range dp {
-		if p.Action == pm.DocumentApprove {
-			user, err := s.User.Get(ctx, p.WhoID)
-			if err == nil {
+	if err != nil {
+		return users, err
+	}
+
+	// all permissions
+	all := sp
+	all = append(all, dp...)
+
+	for _, p := range all {
+		// only approvers
+		if p.Action != pm.DocumentApprove {
+			continue
+		}
+
+		if p.Who == pm.GroupPermission {
+			// get group records for just this group
+			groupRecords := group.FilterGroupRecords(groupMembers, p.WhoID)
+
+			for i := range groupRecords {
+				user, err := s.User.Get(ctx, groupRecords[i].UserID)
+				if err != nil {
+					return users, err
+				}
 				if _, isExisting := prev[user.RefID]; !isExisting {
 					users = append(users, user)
+					prev[user.RefID] = true
 				}
-			} else {
+			}
+		}
+
+		if p.Who == pm.UserPermission {
+			user, err := s.User.Get(ctx, p.WhoID)
+			if err != nil {
 				return users, err
 			}
+
+			if _, isExisting := prev[user.RefID]; !isExisting {
+				users = append(users, user)
+				prev[user.RefID] = true
+			}
 		}
 	}
 
-	return
+	return users, err
 }