From b78e955becf457e3732a359bfc646988878bf09a Mon Sep 17 00:00:00 2001 From: kshitij katiyar <90389917+Kshitij-Katiyar@users.noreply.github.com> Date: Thu, 22 Dec 2022 18:14:39 +0530 Subject: [PATCH] [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 | 5 ++- server/command.go | 51 ++++++++++++---------- server/confluence_config.go | 23 +++++++++- server/kv.go | 10 +++++ server/save_subscription.go | 27 ++++++------ server/serializer/channel_subscription.go | 6 +-- server/serializer/confluence_cloud.go | 2 +- server/serializer/page_subscription.go | 4 +- server/serializer/space_subscription.go | 4 +- server/service/save_subscription.go | 7 +-- server/service/save_subscription_test.go | 8 ++-- server/utils/utils.go | 18 ++++++++ webapp/src/hooks/index.js | 2 +- 13 files changed, 109 insertions(+), 58 deletions(-) diff --git a/assets/templates/command/install_server.md b/assets/templates/command/install_server.md index d549728..a9f7b44 100644 --- a/assets/templates/command/install_server.md +++ b/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. diff --git a/server/command.go b/server/command.go index ed55bb6..ebe0790 100644 --- a/server/command.go +++ b/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 ` 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 ` 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) } diff --git a/server/confluence_config.go b/server/confluence_config.go index 0a1d38b..dbc5b7f 100644 --- a/server/confluence_config.go +++ b/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), } diff --git a/server/kv.go b/server/kv.go index 73c1432..d3b0c4a 100644 --- a/server/kv.go +++ b/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 } diff --git a/server/save_subscription.go b/server/save_subscription.go index 78594c5..2ab1528 100644 --- a/server/save_subscription.go +++ b/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,40 +52,40 @@ 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) @@ -92,17 +93,17 @@ func (p *Plugin) CreateSubscription(body []byte, channelID, subscriptionType, us } 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 diff --git a/server/serializer/channel_subscription.go b/server/serializer/channel_subscription.go index 7c6c5ee..7717677 100644 --- a/server/serializer/channel_subscription.go +++ b/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" ) diff --git a/server/serializer/confluence_cloud.go b/server/serializer/confluence_cloud.go index 8fb7353..214af16 100644 --- a/server/serializer/confluence_cloud.go +++ b/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 { diff --git a/server/serializer/page_subscription.go b/server/serializer/page_subscription.go index 31ec5e6..6232615 100644 --- a/server/serializer/page_subscription.go +++ b/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 diff --git a/server/serializer/space_subscription.go b/server/serializer/space_subscription.go index 1909ad2..ddfc9a6 100644 --- a/server/serializer/space_subscription.go +++ b/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 diff --git a/server/service/save_subscription.go b/server/service/save_subscription.go index 3c55ebf..94e5e3f 100644 --- a/server/service/save_subscription.go +++ b/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() diff --git a/server/service/save_subscription_test.go b/server/service/save_subscription_test.go index d9e5f48..c7884fd 100644 --- a/server/service/save_subscription_test.go +++ b/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{ diff --git a/server/utils/utils.go b/server/utils/utils.go index 3f3b822..1ba3535 100644 --- a/server/utils/utils.go +++ b/server/utils/utils.go @@ -256,3 +256,21 @@ func Map(vs []string, f func(string) string) []string { } return vsm } + +// IsValidURL checks if a given URL is a valid URL with a host and a http or https scheme. +func IsValidURL(rawURL string) error { + u, err := url.ParseRequestURI(rawURL) + if err != nil { + return err + } + + if u.Scheme != "http" && u.Scheme != "https" { + return errors.New("url schema must either be http or https") + } + + if u.Host == "" { + return errors.New("url must contain a host") + } + + return nil +} diff --git a/webapp/src/hooks/index.js b/webapp/src/hooks/index.js index a3a871e..78e8004 100644 --- a/webapp/src/hooks/index.js +++ b/webapp/src/hooks/index.js @@ -26,7 +26,7 @@ export default class Hooks { const state = this.store.getState(); const user = getCurrentUser(state); - if (commandTrimmed && commandTrimmed === '/confluence subscribe') { + if (commandTrimmed && commandTrimmed.startsWith('/confluence subscribe')) { this.store.dispatch(getConnected()); this.store.dispatch(openSubscriptionModal()); return Promise.resolve({});