[MI-2449]:Fixed bugs found by QA (#15)

* [MI-2449]:Fixed bugs found by QA

* [MI-2449]:Fixed lint error

* [MI-2449]:Fixed layout of autosuggest in commands

* [MI-2449]:Fixed review fixes

* [MI-2449]:Fixed test cases

* [MI-2449]:Fixed lint errors

* [MI-2449]:Added proper error

* [MI-2449]:Fixed connect command error

* [MI-2449]:Trim the url in the interactive dialogue

* [MI-2449]:Fixed review fixes

* [MI-2483]:Fixed the URL error in config modal (#17)

* [MI-2483]:Fixed the URL error in config modal

* [MI-2483]:Fixed review fixes

* [MI-2483]:Fixed lint errors

* [MI-2449]

assets/templates/command/install_server.md

@@ -11,7 +11,8 @@
 **Application Permissions**: `Admin`
 Select **Continue**
 5. Copy the `clientID` and `clientSecret` from **Settings**, and paste them into the modal in mattermost which can be opened by using "/confluence config add" slash command.
-6. In Mattermost, use the "/confluence connect" slash command to connect your Mattermost account with your
-   Confluence account.
+6. In Mattermost, use the "/confluence connect ```{{ .ConfluenceURL }}``` admin" slash command to connect your Mattermost account with your confluence admin account and save the token of the admin to handle admin restricted functions.
+7. Use the "/confluence connect" slash command to connect your Mattermost account with your
+Confluence account for all other users.
 
 If you see an option to create a Confluence issue, you're all set! If not, refer to our [documentation](https://mattermost.gitbook.io/plugin-confluence) for troubleshooting help.

server/command.go

@@ -171,22 +171,6 @@ func getAutoCompleteData(showMigrateCommands bool) *model.AutocompleteData {
 	uninstall.AddStaticListArgument("", false, uninstallItems)
 	confluence.AddCommand(uninstall)
 
-	connect := model.NewAutocompleteData("connect", "", "Connect your Mattermost account to your Confluence account")
-	confluence.AddCommand(connect)
-
-	disconnect := model.NewAutocompleteData("disconnect", "", "Disconnect your Mattermost account to your Confluence account")
-	confluence.AddCommand(disconnect)
-
-	list := model.NewAutocompleteData("list", "", "List all subscriptions for the current channel")
-	confluence.AddCommand(list)
-
-	edit := model.NewAutocompleteData("edit", "[name]", "Edit the subscription settings associated with the given subscription name")
-	edit.AddDynamicListArgument("name", "api/v1/autocomplete/channel-subscriptions", false)
-	confluence.AddCommand(edit)
-
-	subscribe := model.NewAutocompleteData("subscribe", "", "Subscribe the current channel to notifications from Confluence")
-	confluence.AddCommand(subscribe)
-
 	config := model.NewAutocompleteData("config", "", "Config related options for confluence instances")
 
 	addConfig := model.NewAutocompleteData("add", "[instance]", "Add config for the confluence instance")
@@ -203,12 +187,25 @@ func getAutoCompleteData(showMigrateCommands bool) *model.AutocompleteData {
 
 	confluence.AddCommand(config)
 
+	connect := model.NewAutocompleteData("connect", "", "Connect your Mattermost account to your Confluence account")
+	confluence.AddCommand(connect)
+
+	disconnect := model.NewAutocompleteData("disconnect", "", "Disconnect your Mattermost account from your Confluence account")
+	confluence.AddCommand(disconnect)
+
+	subscribe := model.NewAutocompleteData("subscribe", "", "Subscribe the current channel to notifications from Confluence")
+	confluence.AddCommand(subscribe)
+
 	unsubscribe := model.NewAutocompleteData("unsubscribe", "[name]", "Unsubscribe the current channel from notifications associated with the given subscription name")
 	unsubscribe.AddDynamicListArgument("name", "api/v1/autocomplete/channel-subscriptions", false)
 	confluence.AddCommand(unsubscribe)
 
-	help := model.NewAutocompleteData("help", "", "Show confluence slash command help")
-	confluence.AddCommand(help)
+	list := model.NewAutocompleteData("list", "", "List all subscriptions for the current channel")
+	confluence.AddCommand(list)
+
+	edit := model.NewAutocompleteData("edit", "[name]", "Edit the subscription settings associated with the given subscription name")
+	edit.AddDynamicListArgument("name", "api/v1/autocomplete/channel-subscriptions", false)
+	confluence.AddCommand(edit)
 
 	if showMigrateCommands {
 		migrate := model.NewAutocompleteData("migrate", "", "Migrate your subscriptions to a newer version of confluence plugin")
@@ -225,6 +222,10 @@ func getAutoCompleteData(showMigrateCommands bool) *model.AutocompleteData {
 		migrate.AddStaticListArgument("", false, migrateItems)
 		confluence.AddCommand(migrate)
 	}
+
+	help := model.NewAutocompleteData("help", "", "Show confluence slash command help")
+	confluence.AddCommand(help)
+
 	return confluence
 }
 
@@ -444,6 +445,10 @@ func deleteConfig(p *Plugin, context *model.CommandArgs, args ...string) *model.
 		return &model.CommandResponse{}
 	}
 
+	if len(args) == 0 {
+		return executeConfluenceDefault(p, context, args...)
+	}
+
 	instance := strings.Join(args, " ")
 
 	if err := p.instanceStore.DeleteInstanceConfig(instance); err != nil {
@@ -640,6 +645,8 @@ func executeConnect(p *Plugin, context *model.CommandArgs, args ...string) *mode
 	if len(args) > 0 {
 		confluenceURL = args[0]
 	}
+
+	confluenceURL = strings.TrimSuffix(confluenceURL, "/")
 	instance := instances.getByAlias(confluenceURL)
 	if instance != nil {
 		confluenceURL = instance.InstanceID.String()
@@ -654,10 +661,10 @@ func executeConnect(p *Plugin, context *model.CommandArgs, args ...string) *mode
 	conn, err := p.userStore.LoadConnection(instanceID, types.ID(context.UserId))
 	if err == nil && len(conn.ConfluenceAccountID()) != 0 {
 		return p.responsef(context,
-			"You already have a Confluence account linked to your Mattermost account from %s. Please use `/confluence disconnect --instance=%s` to disconnect.",
-			instanceID, instanceID)
+			"You already have a Confluence account linked to your Mattermost account from %s. Please use `/confluence disconnect <instance url>` to disconnect.",
+			instanceID)
 	}
-	if _, err = p.instanceStore.LoadInstanceConfig(confluenceURL); err != nil {
+	if _, err = p.instanceStore.LoadInstanceConfig(instanceID.String()); err != nil {
 		return p.responsef(context, configNotFoundError, instanceID, instanceID)
 	}
 
@@ -683,7 +690,7 @@ func executeDisconnect(p *Plugin, commArgs *model.CommandArgs, args ...string) *
 	}
 	disconnected, err := p.DisconnectUser(confluenceURL, types.ID(commArgs.UserId))
 	if errors.Cause(err) == kvstore.ErrNotFound {
-		errorStr := "Your account is not connected to Confluence. Please use `/confluence connect` to connect your account."
+		errorStr := "Your account is not connected to Confluence. Please use `/confluence connect <instance url>` to connect your account."
 		if confluenceURL != "" {
 			errorStr = fmt.Sprintf("You don't have a Confluence account at %s linked to your Mattermost account currently. Please use `/confluence connect` to connect your account.", confluenceURL)
 		}

server/confluence_config.go

@@ -4,15 +4,18 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/gorilla/mux"
 	"github.com/mattermost/mattermost-server/v6/model"
 
 	"github.com/mattermost/mattermost-plugin-confluence/server/serializer"
+	"github.com/mattermost/mattermost-plugin-confluence/server/utils"
 )
 
 const (
-	ParamUserID = "userID"
+	ParamUserID     = "userID"
+	ErrorInvalidURL = "Please enter a valid URL."
 )
 
 func (p *Plugin) handleConfluenceConfig(w http.ResponseWriter, r *http.Request) {
@@ -27,14 +30,30 @@ func (p *Plugin) handleConfluenceConfig(w http.ResponseWriter, r *http.Request)
 
 	decoder := json.NewDecoder(r.Body)
 	submitRequest := &model.SubmitDialogRequest{}
+	response := &model.SubmitDialogResponse{}
 	if err := decoder.Decode(&submitRequest); err != nil {
 		p.API.LogError("Error decoding the submit dialog request.", "Error", err.Error())
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
 
+	configInstanceURL := submitRequest.Submission[configServerURL].(string)
+	if vErr := utils.IsValidURL(configInstanceURL); vErr != nil {
+		response.Error = ErrorInvalidURL
+		response, mErr := json.Marshal(response)
+		if mErr != nil {
+			p.API.LogError("Error in marshaling the response.", "Error", mErr.Error())
+			http.Error(w, mErr.Error(), http.StatusInternalServerError)
+			return
+		}
+
+		p.API.LogError("Error in validating the URL.", "Error", vErr.Error())
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write(response)
+	}
+
 	config := &serializer.ConfluenceConfig{
-		ServerURL:    submitRequest.Submission[configServerURL].(string),
+		ServerURL:    strings.TrimSuffix(configInstanceURL, "/"),
 		ClientID:     submitRequest.Submission[configClientID].(string),
 		ClientSecret: submitRequest.Submission[configClientSecret].(string),
 	}

server/kv.go

@@ -275,6 +275,16 @@ func (store *store) DeleteInstanceConfig(instanceID string) (returnErr error) {
 		returnErr = errors.WithMessage(returnErr,
 			fmt.Sprintf("failed to delete config for instance: %s", instanceID))
 	}()
+
+	config, err := store.plugin.API.KVGet(keyWithInstanceIDForConfig(instanceID))
+	if err != nil {
+		return err
+	}
+
+	if config == nil {
+		return errors.New("config not found for instance")
+	}
+
 	if appErr := store.plugin.API.KVDelete(keyWithInstanceIDForConfig(instanceID)); appErr != nil {
 		return appErr
 	}

server/save_subscription.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/mattermost/mattermost-server/v6/model"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-plugin-confluence/server/config"
 	"github.com/mattermost/mattermost-plugin-confluence/server/serializer"
@@ -28,7 +29,7 @@ func (p *Plugin) handleSaveSubscription(w http.ResponseWriter, r *http.Request)
 
 	body, err := ioutil.ReadAll(r.Body)
 	if err != nil {
-		p.LogAndRespondError(w, http.StatusInternalServerError, "Not able to read the request body", err)
+		p.LogAndRespondError(w, http.StatusInternalServerError, "Not able to read the request body. ", err)
 		return
 	}
 
@@ -51,58 +52,58 @@ func (p *Plugin) handleSaveSubscription(w http.ResponseWriter, r *http.Request)
 
 func (p *Plugin) LogAndRespondError(w http.ResponseWriter, statusCode int, errorLog string, err error) {
 	p.API.LogError(errorLog, "Error", err.Error())
-	http.Error(w, errorLog, statusCode)
+	http.Error(w, errors.WithMessage(err, errorLog).Error(), statusCode)
 }
 
 func (p *Plugin) CreateSubscription(body []byte, channelID, subscriptionType, userID, path string) (int, string, error) {
 	instance, err := p.getInstanceFromURL(path)
 	if err != nil {
-		return http.StatusInternalServerError, "Not able to get instance from url", err
+		return http.StatusInternalServerError, "Not able to get instance from url. ", err
 	}
 
 	if err = p.HasPermissionToManageSubscription(instance.GetURL(), userID, channelID); err != nil {
-		return http.StatusForbidden, "You don't have the permission to create a subscription. Please contact your administrator.", err
+		return http.StatusForbidden, "You don't have the permission to create a subscription. Please contact your administrator. ", err
 	}
 
 	conn, err := p.userStore.LoadConnection(types.ID(instance.GetURL()), types.ID(userID))
 	if err != nil {
-		return http.StatusInternalServerError, "Error in loading connection.", err
+		return http.StatusInternalServerError, "Error in loading connection. ", err
 	}
 
 	client, err := instance.GetClient(conn)
 	if err != nil {
-		return http.StatusInternalServerError, "Not able to get Client.", err
+		return http.StatusInternalServerError, "Not able to get Client. ", err
 	}
 
 	var subscription serializer.Subscription
 	if subscriptionType == serializer.SubscriptionTypeSpace {
 		subscription, err = serializer.SpaceSubscriptionFromJSON(body)
 		if err != nil {
-			return http.StatusBadRequest, "Error decoding request body for space subscription.", err
+			return http.StatusBadRequest, "Error decoding request body for space subscription. ", err
 		}
 
 		spaceKey := subscription.(*serializer.SpaceSubscription).GetSubscription().SpaceKey
 		resp, gErr := client.GetSpaceData(spaceKey)
 		if gErr != nil {
-			return http.StatusBadRequest, "Error getting space related data for space subscription.", gErr
+			return http.StatusBadRequest, "Error getting space related data for space subscription. ", gErr
 		}
 
 		updatedSubscrption := subscription.(*serializer.SpaceSubscription).GetSubscription().UpdateSpaceIDAndUserID(strconv.FormatInt(resp.ID, 10), userID)
 		subscription = updatedSubscrption.GetSubscription()
 	} else if subscriptionType == serializer.SubscriptionTypePage {
 		subscription, err = serializer.PageSubscriptionFromJSON(body)
 		if err != nil {
-			return http.StatusBadRequest, "Error decoding request body for page subscription.", err
+			return http.StatusBadRequest, "Error decoding request body for page subscription. ", err
 		}
 
 		pageID, err := strconv.Atoi(subscription.(*serializer.PageSubscription).GetSubscription().PageID)
 		if err != nil {
-			return http.StatusInternalServerError, "Error converting pageID to integer.", err
+			return http.StatusInternalServerError, "Error converting pageID to integer. ", err
 		}
 
 		_, err = client.GetPageData(pageID)
 		if err != nil {
-			return http.StatusInternalServerError, "Error getting page related data for page subscription.", err
+			return http.StatusInternalServerError, "Error getting page related data for page subscription. ", err
 		}
 
 		updatedSubscrption := subscription.(*serializer.PageSubscription).UpdateUserID(userID)
@@ -111,13 +112,13 @@ func (p *Plugin) CreateSubscription(body []byte, channelID, subscriptionType, us
 
 	if instance.Common().Type == ServerInstanceType {
 		if err := p.CreateWebhook(instance, subscription, userID); err != nil {
-			return http.StatusBadRequest, "Not able to create webhook.", err
+			return http.StatusBadRequest, "Not able to create webhook. ", err
 		}
 	}
 
 	statusCode, sErr := service.SaveSubscription(subscription)
 	if sErr != nil {
-		return statusCode, "Not able to save the subscription", sErr
+		return statusCode, "Not able to save the subscription. ", sErr
 	}
 
 	return statusCode, subscriptionSaveSuccess, nil

server/serializer/channel_subscription.go

@@ -21,9 +21,9 @@ const (
 	SubscriptionTypeSpace = "space_subscription"
 	SubscriptionTypePage  = "page_subscription"
 
-	aliasAlreadyExist       = "a subscription with the same name already exists in this channel"
-	urlSpaceKeyAlreadyExist = "a subscription with the same url and space key already exists in this channel"
-	urlPageIDAlreadyExist   = "a subscription with the same url and page id already exists in this channel"
+	AliasAlreadyExist       = "subscription with the same name already exists in this channel"
+	URLSpaceKeyAlreadyExist = "subscription with the same URL and space key already exists in this channel"
+	URLPageIDAlreadyExist   = "subscription with the same URL and page id already exists in this channel"
 	subscriptionFormatter   = "\n\n"
 )
 

server/serializer/confluence_cloud.go

@@ -97,7 +97,7 @@ func (e ConfluenceCloudEvent) GetNotificationPost(eventType, baseURL, botUserID
 				Fallback:  message,
 				Pretext:   message,
 				Title:     e.Page.Title,
-				TitleLink: fmt.Sprintf("%s/%s", baseURL, e.Page.Self),
+				TitleLink: e.Page.Self,
 				Text:      fmt.Sprintf("%s\n\n[**View in Confluence**](%s)", strings.TrimSpace(e.Page.Body.View.Value), e.Page.Self),
 			}
 		} else {

server/serializer/page_subscription.go

@@ -153,13 +153,13 @@ func (ps *PageSubscription) ValidateSubscription(subs *Subscriptions) error {
 	}
 	if channelSubscriptions, valid := subs.ByChannelID[ps.ChannelID]; valid {
 		if _, ok := channelSubscriptions[ps.Alias]; ok {
-			return errors.New(aliasAlreadyExist)
+			return errors.New(AliasAlreadyExist)
 		}
 	}
 	key := store.GetURLPageIDCombinationKey(ps.BaseURL, ps.PageID)
 	if urlPageIDSubscriptions, valid := subs.ByURLPageID[key]; valid {
 		if _, ok := urlPageIDSubscriptions[ps.ChannelID]; ok {
-			return errors.New(urlPageIDAlreadyExist)
+			return errors.New(URLPageIDAlreadyExist)
 		}
 	}
 	return nil

server/serializer/space_subscription.go

@@ -162,13 +162,13 @@ func (ss *SpaceSubscription) ValidateSubscription(subs *Subscriptions) error {
 	}
 	if channelSubscriptions, valid := subs.ByChannelID[ss.ChannelID]; valid {
 		if _, ok := channelSubscriptions[ss.Alias]; ok {
-			return errors.New(aliasAlreadyExist)
+			return errors.New(AliasAlreadyExist)
 		}
 	}
 	key := store.GetURLSpaceKeyCombinationKey(ss.BaseURL, ss.SpaceKey)
 	if urlSpaceKeySubscriptions, valid := subs.ByURLSpaceKey[key]; valid {
 		if _, ok := urlSpaceKeySubscriptions[ss.ChannelID]; ok {
-			return errors.New(urlSpaceKeyAlreadyExist)
+			return errors.New(URLSpaceKeyAlreadyExist)
 		}
 	}
 	return nil

server/service/save_subscription.go

@@ -9,12 +9,7 @@ import (
 	"github.com/mattermost/mattermost-plugin-confluence/server/store"
 )
 
-const (
-	generalSaveError        = "an error occurred attempting to save a subscription"
-	aliasAlreadyExist       = "a subscription with the same name already exists in this channel"
-	urlSpaceKeyAlreadyExist = "a subscription with the same url and space key already exists in this channel"
-	urlPageIDAlreadyExist   = "a subscription with the same url and page id already exists in this channel"
-)
+const generalSaveError = "an error occurred while attempting to save a subscription"
 
 func SaveSubscription(subscription serializer.Subscription) (int, error) {
 	subs, err := GetSubscriptions()

server/service/save_subscription_test.go

@@ -28,7 +28,7 @@ func TestSaveSpaceSubscription(t *testing.T) {
 				},
 			},
 			statusCode:   http.StatusBadRequest,
-			errorMessage: aliasAlreadyExist,
+			errorMessage: serializer.AliasAlreadyExist,
 		},
 		"url space key combination already exist": {
 			newSubscription: &serializer.SpaceSubscription{
@@ -41,7 +41,7 @@ func TestSaveSpaceSubscription(t *testing.T) {
 				},
 			},
 			statusCode:   http.StatusBadRequest,
-			errorMessage: urlSpaceKeyAlreadyExist,
+			errorMessage: serializer.URLSpaceKeyAlreadyExist,
 		},
 		"subscription unique base url": {
 			newSubscription: &serializer.SpaceSubscription{
@@ -144,7 +144,7 @@ func TestSavePageSubscription(t *testing.T) {
 				},
 			},
 			statusCode:   http.StatusBadRequest,
-			errorMessage: aliasAlreadyExist,
+			errorMessage: serializer.AliasAlreadyExist,
 		},
 		"url page id combination already exist": {
 			newSubscription: &serializer.PageSubscription{
@@ -157,7 +157,7 @@ func TestSavePageSubscription(t *testing.T) {
 				},
 			},
 			statusCode:   http.StatusBadRequest,
-			errorMessage: urlPageIDAlreadyExist,
+			errorMessage: serializer.URLPageIDAlreadyExist,
 		},
 		"subscription unique base url": {
 			newSubscription: &serializer.PageSubscription{