[MI-2669]: Made changes for subscriptionID to be used as key for KV-s… (

#33)

* [MI-2669]: Made changes for subscriptionID to be used as key for KV-store, sorted subscriptions on basis of createdAt, users cannot see subscriptions of not participating channels

* [MI-2669]: Modified comment

---------

Co-authored-by: Abhishek Verma <abhishek.verma@brightscout.com>

server/plugin/api.go

@@ -123,27 +123,11 @@ func (p *Plugin) handleLink(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	isAdmin := false
-	subscriptionStatusCode, subscriptionErr := p.Client.CheckIfUserIsProjectAdmin(body.Organization, response.ID, p.GetPluginURL(), mattermostUserID)
-	if subscriptionErr != nil {
-		switch {
-		case subscriptionStatusCode == http.StatusBadRequest && strings.Contains(subscriptionErr.Error(), fmt.Sprintf(constants.ErrorMessageForAdmin, constants.SubscriptionEventTypeDummy)):
-			isAdmin = true
-		case subscriptionStatusCode == http.StatusForbidden && strings.Contains(subscriptionErr.Error(), constants.AccessDenied):
-			isAdmin = false
-		default:
-			p.API.LogError(fmt.Sprintf(constants.ErrorCheckingProjectAdmin, body.Project), "Error", subscriptionErr.Error())
-			p.handleError(w, r, &serializers.Error{Code: subscriptionStatusCode, Message: constants.ErrorLinkProject})
-			return
-		}
-	}
-
 	project := serializers.ProjectDetails{
 		MattermostUserID: mattermostUserID,
 		ProjectID:        response.ID,
 		ProjectName:      cases.Title(language.Und).String(body.Project),
 		OrganizationName: strings.ToLower(body.Organization),
-		IsAdmin:          isAdmin,
 	}
 
 	if storeErr := p.Store.StoreProject(&project); storeErr != nil {
@@ -301,10 +285,11 @@ func (p *Plugin) handleCreateSubscription(w http.ResponseWriter, r *http.Request
 	}
 
 	if _, isSubscriptionPresent := p.IsSubscriptionPresent(subscriptionList, &serializers.SubscriptionDetails{
-		OrganizationName:             body.Organization,
-		ProjectName:                  body.Project,
-		ChannelID:                    body.ChannelID,
-		EventType:                    body.EventType,
+		OrganizationName: body.Organization,
+		ProjectName:      body.Project,
+		ChannelID:        body.ChannelID,
+		EventType:        body.EventType,
+		// Below all are filters that could be present on different categories of subscriptions from Boards, Repos and Pipelines
 		Repository:                   body.Repository,
 		TargetBranch:                 body.TargetBranch,
 		PullRequestCreatedBy:         body.PullRequestCreatedBy,
@@ -360,17 +345,18 @@ func (p *Plugin) handleCreateSubscription(w http.ResponseWriter, r *http.Request
 		createdByDisplayName = user.Username // If user's first/last name doesn't exist then show username as fallback
 	}
 	if storeErr := p.Store.StoreSubscription(&serializers.SubscriptionDetails{
-		MattermostUserID:                 mattermostUserID,
-		ProjectName:                      body.Project,
-		ProjectID:                        project.ProjectID,
-		OrganizationName:                 body.Organization,
-		EventType:                        body.EventType,
-		ServiceType:                      body.ServiceType,
-		ChannelID:                        body.ChannelID,
-		SubscriptionID:                   subscription.ID,
-		ChannelName:                      channel.DisplayName,
-		ChannelType:                      channel.Type,
-		CreatedBy:                        strings.TrimSpace(createdByDisplayName),
+		MattermostUserID: mattermostUserID,
+		ProjectName:      body.Project,
+		ProjectID:        project.ProjectID,
+		OrganizationName: body.Organization,
+		EventType:        body.EventType,
+		ServiceType:      body.ServiceType,
+		ChannelID:        body.ChannelID,
+		SubscriptionID:   subscription.ID,
+		ChannelName:      channel.DisplayName,
+		ChannelType:      channel.Type,
+		CreatedBy:        strings.TrimSpace(createdByDisplayName),
+		// Below all are filters that could be present on different categories of subscriptions from Boards, Repos and Pipelines
 		Repository:                       body.Repository,
 		TargetBranch:                     body.TargetBranch,
 		RepositoryName:                   body.RepositoryName,
@@ -495,62 +481,7 @@ func (p *Plugin) handleGetSubscriptions(w http.ResponseWriter, r *http.Request)
 		}
 
 		sort.Slice(subscriptionByProject, func(i, j int) bool {
-			return subscriptionByProject[i].ChannelName+
-				subscriptionByProject[i].EventType+
-				subscriptionByProject[i].TargetBranch+
-				subscriptionByProject[i].PullRequestCreatedByName+
-				subscriptionByProject[i].PullRequestReviewersContainsName+
-				subscriptionByProject[i].PushedByName+
-				subscriptionByProject[i].MergeResultName+
-				subscriptionByProject[i].NotificationTypeName+
-				subscriptionByProject[i].AreaPath+
-				subscriptionByProject[i].ReleasePipelineName+
-				subscriptionByProject[i].BuildPipeline+
-				subscriptionByProject[i].BuildStatusName+
-				subscriptionByProject[i].ApprovalStatusName+
-				subscriptionByProject[i].ApprovalTypeName+
-				subscriptionByProject[i].StageNameValue+
-				subscriptionByProject[i].ReleaseStatusName+
-				subscriptionByProject[i].RunPipeline+
-				subscriptionByProject[i].RunPipelineName+
-				subscriptionByProject[i].ReleasePipelineName+
-				subscriptionByProject[i].RunStageName+
-				subscriptionByProject[i].RunEnvironmentName+
-				subscriptionByProject[i].RunStageNameID+
-				subscriptionByProject[i].RunStageStateID+
-				subscriptionByProject[i].RunStageStateIDName+
-				subscriptionByProject[i].RunStageResultID+
-				subscriptionByProject[i].RunStateID+
-				subscriptionByProject[i].RunStateIDName+
-				subscriptionByProject[i].RunResultID <
-				subscriptionByProject[j].ChannelName+
-					subscriptionByProject[j].EventType+
-					subscriptionByProject[j].TargetBranch+
-					subscriptionByProject[j].PullRequestCreatedByName+
-					subscriptionByProject[j].PullRequestReviewersContainsName+
-					subscriptionByProject[j].PushedByName+
-					subscriptionByProject[j].MergeResultName+
-					subscriptionByProject[j].NotificationTypeName+
-					subscriptionByProject[j].AreaPath+
-					subscriptionByProject[i].ReleasePipelineName+
-					subscriptionByProject[i].BuildPipeline+
-					subscriptionByProject[i].BuildStatusName+
-					subscriptionByProject[i].ApprovalStatusName+
-					subscriptionByProject[i].ApprovalTypeName+
-					subscriptionByProject[i].StageNameValue+
-					subscriptionByProject[i].ReleaseStatusName+
-					subscriptionByProject[i].RunPipeline+
-					subscriptionByProject[i].RunPipelineName+
-					subscriptionByProject[i].ReleasePipelineName+
-					subscriptionByProject[i].RunStageName+
-					subscriptionByProject[i].RunEnvironmentName+
-					subscriptionByProject[i].RunStageNameID+
-					subscriptionByProject[i].RunStageStateID+
-					subscriptionByProject[i].RunStageStateIDName+
-					subscriptionByProject[i].RunStageResultID+
-					subscriptionByProject[i].RunStateID+
-					subscriptionByProject[i].RunStateIDName+
-					subscriptionByProject[i].RunResultID
+			return subscriptionByProject[i].CreatedAt.After(subscriptionByProject[j].CreatedAt)
 		})
 
 		filteredSubscriptionList, filteredSubscriptionErr := p.GetSubscriptionsForAccessibleChannelsOrProjects(subscriptionByProject, teamID, mattermostUserID, constants.FilterCreatedByAnyone)

server/plugin/api_test.go

@@ -279,7 +279,6 @@ func TestHandleLink(t *testing.T) {
 				mockedStore.EXPECT().GetAllProjects(testutils.MockMattermostUserID).Return(testCase.projectList, nil)
 				if !testCase.isProjectLinked {
 					mockedClient.EXPECT().Link(gomock.Any(), gomock.Any()).Return(&serializers.Project{}, testCase.statusCode, testCase.err)
-					mockedClient.EXPECT().CheckIfUserIsProjectAdmin(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(http.StatusOK, nil)
 					mockedStore.EXPECT().StoreProject(&serializers.ProjectDetails{
 						MattermostUserID: testutils.MockMattermostUserID,
 						ProjectName:      "Mockproject",

server/plugin/client.go

@@ -24,7 +24,6 @@ type Client interface {
 	Link(body *serializers.LinkRequestPayload, mattermostUserID string) (*serializers.Project, int, error)
 	CreateSubscription(body *serializers.CreateSubscriptionRequestPayload, project *serializers.ProjectDetails, channelID, pluginURL, mattermostUserID string) (*serializers.SubscriptionValue, int, error)
 	DeleteSubscription(organization, subscriptionID, mattermostUserID string) (int, error)
-	CheckIfUserIsProjectAdmin(organizationName, projectID, pluginURL, mattermostUserID string) (int, error)
 	UpdatePipelineApprovalRequest(pipelineApproveRequestPayload *serializers.PipelineApproveRequest, organization, projectName, mattermostUserID string, approvalID int) (int, error)
 	UpdatePipelineRunApprovalRequest(pipelineApproveRequestPayload []*serializers.PipelineApproveRequest, organization, projectID, mattermostUserID string) (*serializers.PipelineRunApproveResponse, int, error)
 	GetApprovalDetails(organization, projectName, mattermostUserID string, approvalID int) (*serializers.PipelineApprovalDetails, int, error)
@@ -250,37 +249,6 @@ func (c *client) CreateSubscription(body *serializers.CreateSubscriptionRequestP
 	return subscription, statusCode, nil
 }
 
-// We are passing an invalid request payload for creating a subscription to check if the user who is linking this project is an admin here or not
-// If the user is an admin then we will get a response status code 400 with a message of invalid payload
-// and if the user is not an admin we will get status code 403 with a message saying "Access Denied"
-func (c *client) CheckIfUserIsProjectAdmin(organizationName, projectID, pluginURL, mattermostUserID string) (int, error) {
-	subscriptionURL := fmt.Sprintf(constants.CreateSubscription, organizationName)
-
-	publisherInputs := serializers.PublisherInputsGeneric{
-		ProjectID: projectID,
-	}
-
-	consumerInputs := serializers.ConsumerInputs{
-		URL: fmt.Sprintf("%s%s?channelID=%s", strings.TrimRight(pluginURL, "/"), constants.PathSubscriptionNotifications, constants.SubscriptionEventTypeDummy),
-	}
-
-	payload := serializers.CreateSubscriptionBodyPayload{
-		PublisherID:      constants.PublisherIDTFS,
-		EventType:        constants.SubscriptionEventTypeDummy,
-		ConsumerID:       constants.ConsumerID,
-		ConsumerActionID: constants.ConsumerActionID,
-		PublisherInputs:  publisherInputs,
-		ConsumerInputs:   consumerInputs,
-	}
-
-	_, statusCode, err := c.CallJSON(c.plugin.getConfiguration().AzureDevopsAPIBaseURL, subscriptionURL, http.MethodPost, mattermostUserID, payload, nil, nil)
-	if err != nil {
-		return statusCode, errors.Wrap(err, "failed to check if user is a project admin")
-	}
-
-	return statusCode, nil
-}
-
 func (c *client) DeleteSubscription(organization, subscriptionID, mattermostUserID string) (int, error) {
 	subscriptionURL := fmt.Sprintf(constants.DeleteSubscription, organization, subscriptionID)
 	_, statusCode, err := c.CallJSON(c.plugin.getConfiguration().AzureDevopsAPIBaseURL, subscriptionURL, http.MethodDelete, mattermostUserID, nil, nil, nil)

server/plugin/client_test.go

@@ -571,44 +571,6 @@ func TestGetApprovalDetails(t *testing.T) {
 	}
 }
 
-func TestCheckIfUserIsProjectAdmin(t *testing.T) {
-	defer monkey.UnpatchAll()
-	p := setupTestPlugin(&plugintest.API{})
-	for _, testCase := range []struct {
-		description   string
-		err           error
-		statusCode    int
-		expectedError string
-	}{
-		{
-			description: "CheckIfUserIsProjectAdmin: valid",
-			statusCode:  http.StatusOK,
-		},
-		{
-			description:   "CheckIfUserIsProjectAdmin: with error",
-			err:           errors.New("failed to check user permissions"),
-			statusCode:    http.StatusInternalServerError,
-			expectedError: "failed to check if user is a project admin: failed to check user permissions",
-		},
-	} {
-		t.Run(testCase.description, func(t *testing.T) {
-			monkey.PatchInstanceMethod(reflect.TypeOf(&client{}), "Call", func(_ *client, basePath, method, path, contentType, mattermostUserID string, inBody io.Reader, out interface{}, formValues url.Values) (responseData []byte, statusCode int, err error) {
-				return nil, testCase.statusCode, testCase.err
-			})
-
-			statusCode, err := p.Client.CheckIfUserIsProjectAdmin(testutils.MockOrganization, testutils.MockProjectID, "mockProjectURL", testutils.MockMattermostUserID)
-
-			if testCase.err != nil {
-				assert.EqualError(t, err, testCase.expectedError)
-			} else {
-				assert.NoError(t, err)
-			}
-
-			assert.Equal(t, testCase.statusCode, statusCode)
-		})
-	}
-}
-
 func TestGetSubscriptionFilterPossibleValues(t *testing.T) {
 	defer monkey.UnpatchAll()
 	p := setupTestPlugin(&plugintest.API{})

server/plugin/utils.go

@@ -334,12 +334,6 @@ func (p *Plugin) GetSubscriptionsForAccessibleChannelsOrProjects(subscriptionLis
 		return nil, channelErr
 	}
 
-	projectList, err := p.Store.GetAllProjects(mattermostUserID)
-	if err != nil {
-		p.API.LogError(constants.ErrorFetchProjectList, "Error", err.Error())
-		return nil, err
-	}
-
 	var filteredSubscriptionList []*serializers.SubscriptionDetails
 	if createdBy == constants.FilterCreatedByMe {
 		for _, subscription := range subscriptionList {
@@ -350,21 +344,13 @@ func (p *Plugin) GetSubscriptionsForAccessibleChannelsOrProjects(subscriptionLis
 		return filteredSubscriptionList, nil
 	}
 
+	// For each subscription check if the user is a member of the MM channel where the subscription is created
 	for _, subscription := range subscriptionList {
-		// For each subscription on a project check if a user is an admin or member of the MM channel to return subscriptions
-		if projectDetails, isProjectLinked := p.IsProjectLinked(projectList, serializers.ProjectDetails{OrganizationName: subscription.OrganizationName, ProjectName: subscription.ProjectName}); isProjectLinked {
-			if projectDetails.IsAdmin {
+		for _, channel := range channels {
+			if subscription.ChannelID == channel.Id {
 				filteredSubscriptionList = append(filteredSubscriptionList, subscription)
-			} else {
-				for _, channel := range channels {
-					if subscription.ChannelID == channel.Id {
-						filteredSubscriptionList = append(filteredSubscriptionList, subscription)
-						break
-					}
-				}
+				break
 			}
-		} else if subscription.MattermostUserID == mattermostUserID {
-			filteredSubscriptionList = append(filteredSubscriptionList, subscription)
 		}
 	}
 

server/serializers/project.go

@@ -13,7 +13,6 @@ type ProjectDetails struct {
 	ProjectID           string `json:"projectID"`
 	ProjectName         string `json:"projectName"`
 	OrganizationName    string `json:"organizationName"`
-	IsAdmin             bool   `json:"isAdmin"`
 	DeleteSubscriptions bool   `json:"deleteSubscriptions"`
 }
 

server/serializers/subscriptions.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"time"
 
 	"github.com/mattermost/mattermost-plugin-azure-devops/server/constants"
 )
@@ -152,17 +153,19 @@ type CreateSubscriptionBodyPayload struct {
 }
 
 type SubscriptionDetails struct {
-	MattermostUserID                 string `json:"mattermostUserID"`
-	ProjectName                      string `json:"projectName"`
-	ProjectID                        string `json:"projectID"`
-	OrganizationName                 string `json:"organizationName"`
-	EventType                        string `json:"eventType"`
-	ServiceType                      string `json:"serviceType"`
-	ChannelID                        string `json:"channelID"`
-	ChannelName                      string `json:"channelName"`
-	ChannelType                      string `json:"channelType"`
-	SubscriptionID                   string `json:"subscriptionID"`
-	CreatedBy                        string `json:"createdBy"`
+	MattermostUserID string    `json:"mattermostUserID"`
+	ProjectName      string    `json:"projectName"`
+	ProjectID        string    `json:"projectID"`
+	OrganizationName string    `json:"organizationName"`
+	EventType        string    `json:"eventType"`
+	ServiceType      string    `json:"serviceType"`
+	ChannelID        string    `json:"channelID"`
+	ChannelName      string    `json:"channelName"`
+	ChannelType      string    `json:"channelType"`
+	SubscriptionID   string    `json:"subscriptionID"`
+	CreatedBy        string    `json:"createdBy"`
+	CreatedAt        time.Time `json:"createdAt"`
+	// Below all are filters that could be present on different categories of subscriptions from Boards, Repos and Pipelines
 	TargetBranch                     string `json:"targetBranch"`
 	Repository                       string `json:"repository"`
 	RepositoryName                   string `json:"repositoryName"`

server/store/link.go

@@ -62,7 +62,6 @@ func (projectList *ProjectList) AddProject(userID string, project *serializers.P
 		ProjectID:        project.ProjectID,
 		ProjectName:      project.ProjectName,
 		OrganizationName: project.OrganizationName,
-		IsAdmin:          project.IsAdmin,
 	}
 	projectList.ByMattermostUserID[userID][projectKey] = projectListValue
 }