Convert Autolink plugin into an App, and introduce autolink edit modal #159

mickmister · 2021-04-26T04:51:09Z

Summary

One thing not done here is having the plugin automatically register itself as an app. I'm not sure how that would work with admin interaction. Right now you have to run

/apps debug-add-manifest http://localhost:8065/plugins/mattermost-autolink/app/v1/manifest
/apps install --app-id com.mattermost.autolink

Then there are two autolink commands:

The existing autolink command
A new autolink-app edit command to open the modal

Added features:

Renaming auto links
Deleting auto links
Testing auto links within the modal

Ticket Link

Fixes #133

Related Pull Requests

mattermost/mattermost-plugin-apps#165

Screenshots

handle clearing the selected auto link

codecov-commenter · 2021-04-26T04:51:14Z

Codecov Report

Merging #159 (d92a803) into master (a868350) will decrease coverage by 0.46%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   40.03%   39.57%   -0.47%     
==========================================
  Files           6        6              
  Lines         602      609       +7     
==========================================
  Hits          241      241              
- Misses        345      352       +7     
  Partials       16       16

Impacted Files	Coverage Δ
server/api/api.go	`58.33% <ø> (-2.96%)`	⬇️
server/autolinkplugin/plugin.go	`86.77% <40.00%> (-4.30%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a868350...d92a803. Read the comment docs.

hanzei

Great work. It's really exiting to see what app can enable in terms of UX possibilities.

A few comments below.

hanzei · 2021-04-26T07:04:35Z

Makefile

@@ -92,7 +92,7 @@ ifneq ($(wildcard $(ASSETS_DIR)/.),)
 	cp -r $(ASSETS_DIR) dist/$(PLUGIN_ID)/
 endif
 ifneq ($(HAS_PUBLIC),)
-	cp -r public/ dist/$(PLUGIN_ID)/
+	cp -r public/ dist/$(PLUGIN_ID)/public


Let's stick with the starter template

Suggested change

cp -r public/ dist/$(PLUGIN_ID)/public

cp -r public dist/$(PLUGIN_ID)/

@hanzei The code was originally putting the icon.png file into the dist folder, and not in the dist/public folder. This was how I was able to get the icon in the right folder.

You are using a mac, right? Could you please test my code?

server/autolinkapp/app.go

hanzei · 2021-04-26T07:10:45Z

server/autolinkapp/app.go

+func (a *app) handleIcon(w http.ResponseWriter, r *http.Request) {
+	resp := a.getEmptyFormResponse()
+	httputils.WriteJSON(w, resp)
+}


hanzei · 2021-04-26T07:11:29Z

server/autolinkapp/app.go

+	adminRoutes.HandleFunc(bindingsRoute, a.handleBindings).Methods(http.MethodPost)
+	adminRoutes.HandleFunc(editLinksRoute+"/form", a.handleFormFetch).Methods(http.MethodPost)
+	adminRoutes.HandleFunc(editLinksRoute+"/submit", a.handleFormSubmit).Methods(http.MethodPost)
+	adminRoutes.HandleFunc(editLinksCommandRoute+"/submit", a.handleCommandSubmit).Methods(http.MethodPost)


Is this really /edit-links-command/submit/submit?

Hmm that was happening before and I thought I fixed that. Not sure how/why it's working

hanzei · 2021-04-26T07:11:58Z

server/autolinkapp/app.go

+		}
+
+		r.Body.Close()
+		r.Body = ioutil.NopCloser(bytes.NewBuffer(b))


Why is this needed?

I'm reading from the body in this middleware, and want the http handlers to be able to read from the body as well. I don't like this solution

Is the approach from servicenow suitable here? https://github.com/mattermost/mattermost-app-servicenow/blob/5c160ea3d7c07017a5b1327c1a3c0a9ec5062c53/routers/mattermost/mattermost.go#L43-L66

hanzei · 2021-04-26T07:14:29Z

server/autolinkapp/form.go

+		linkOption = values.Link
+	}
+
+	return &apps.Form{


server/autolinkapp/form.go

hanzei · 2021-04-26T07:19:48Z

server/autolinkapp/form.go

+			},
+			{
+				Name:       fieldScope,
+				ModalLabel: "Scope",


Out of scope of this PR: This should be autocompleted

I'm not sure how that would be autocompleted. Can you explain further?

Scope is either a channel or a team.

aaronrothschild · 2021-04-26T14:24:10Z

Sorry, maybe I missed something - I thought apps don't have the "MessageWillBePosted" hook. Is this operating on the "MessageHasBeenPsted" hook and then editing the message retroactively? If so, it's a bit different and worth documenting since in secure environments the data will have been committed to the DB layer and it may be part of the audit record, etc. Whereas, I believe the plugin Autolink would intercept the message before it is recorded anywhere.

If so, we should document the slight difference for admins in the readme, that's all. Great stuff!

aaronrothschild

LGTM

mickmister · 2021-04-26T15:25:56Z

@aaronrothschild The plugin will still be doing the string substitution with MessageWillBePosted. The "App" is embedded in the plugin, and is currently just used for this modal.

levb

I have not yet looked into the details of the app implementation, but a suggestion right away. Think big.

In Apps: add an Epic "Apps to support plugins"
Add a Plugin Upstream (server/upstream/plugin) that will invoke using the P2P "HTTP" interface
Add [Un-]Install REST API to plugin-apps
In Autolink:
1./autolink experimental as-app on/off that would install itself using the API and unregister the old style command

levb · 2021-04-29T17:29:17Z

go.mod

-	github.com/gorilla/mux v1.7.4
-	github.com/mattermost/mattermost-server/v5 v5.28.1
-	github.com/mholt/archiver/v3 v3.3.0
+	github.com/go-sql-driver/mysql v1.6.0 // indirect


Is this tidy-ed?

mickmister · 2021-10-08T16:01:19Z

@hanzei I'm wanting to close this to clean up my open PR queue. What are your thoughts on this?

hanzei · 2021-10-10T00:34:12Z

Would be recommend customers to use this setting? Would be we fine to break it by introducing breaking changes in the Apps framework?

cc @aaronrothschild

mickmister · 2021-10-11T00:51:11Z

Would be recommend customers to use this setting?

@hanzei The feature received great positive feedback in the R&D demo, so I would imagine that it would be used by us as an organization.

Would be we fine to break it by introducing breaking changes in the Apps framework?

I think if we introduce it as an experimental feature it would be fine.

To clear up my original request, I was saying that we close this PR in favor of your PR #164. We can also merge your PR into this PR and continue here.

levb · 2021-10-11T14:00:26Z

This is _very_experimental to add to one of the most used plugins...

aaronrothschild · 2021-10-11T21:32:28Z

This is _very_experimental to add to one of the most used plugins...

I agree with Lev....this probably can go out later when the App framework is GA...

mickmister · 2021-10-15T03:51:51Z

Closing for now. Will re-open when we want to properly implement the feature on a stable Apps release

mickmister added 3 commits April 23, 2021 20:59

WIP transition into App

49e7b07

implement autolink crud

beeed35

move form submit handlers to their own file

0991d59

handle clearing the selected auto link

mickmister added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 26, 2021

mickmister requested review from aaronrothschild, levb, hanzei and DHaussermann April 26, 2021 04:51

mickmister mentioned this pull request Apr 26, 2021

Introduce ReadOnly property for form fields mattermost/mattermost-plugin-apps#165

Merged

hanzei requested changes Apr 26, 2021

View reviewed changes

aaronrothschild approved these changes Apr 26, 2021

View reviewed changes

fix typo

d92a803

levb suggested changes Apr 29, 2021

View reviewed changes

mickmister closed this Oct 15, 2021

This was referenced Oct 19, 2021

Make use of Apps support for Plugins (master as base branch) #170

Closed

Make use of Apps support for Plugins #164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Autolink plugin into an App, and introduce autolink edit modal #159

Convert Autolink plugin into an App, and introduce autolink edit modal #159

mickmister commented Apr 26, 2021 •

edited

Loading

codecov-commenter commented Apr 26, 2021 •

edited

Loading

hanzei left a comment

hanzei Apr 26, 2021

mickmister Apr 26, 2021

hanzei Apr 27, 2021

hanzei Apr 26, 2021

hanzei Apr 26, 2021

mickmister Apr 26, 2021

hanzei Apr 26, 2021

mickmister Apr 26, 2021

hanzei Apr 27, 2021

hanzei Apr 26, 2021

hanzei Apr 26, 2021

mickmister Apr 26, 2021

hanzei Apr 27, 2021

aaronrothschild commented Apr 26, 2021

aaronrothschild left a comment

mickmister commented Apr 26, 2021

levb left a comment

levb Apr 29, 2021

mickmister commented Oct 8, 2021

hanzei commented Oct 10, 2021

mickmister commented Oct 11, 2021

levb commented Oct 11, 2021

aaronrothschild commented Oct 11, 2021

mickmister commented Oct 15, 2021

	cp -r public/ dist/$(PLUGIN_ID)/public
	cp -r public dist/$(PLUGIN_ID)/

Convert Autolink plugin into an App, and introduce autolink edit modal #159

Convert Autolink plugin into an App, and introduce autolink edit modal #159

Conversation

mickmister commented Apr 26, 2021 • edited Loading

Summary

Ticket Link

Related Pull Requests

Screenshots

codecov-commenter commented Apr 26, 2021 • edited Loading

Codecov Report

hanzei 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

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

aaronrothschild commented Apr 26, 2021

aaronrothschild left a comment

Choose a reason for hiding this comment

mickmister commented Apr 26, 2021

levb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister commented Oct 8, 2021

hanzei commented Oct 10, 2021

mickmister commented Oct 11, 2021

levb commented Oct 11, 2021

aaronrothschild commented Oct 11, 2021

mickmister commented Oct 15, 2021

mickmister commented Apr 26, 2021 •

edited

Loading

codecov-commenter commented Apr 26, 2021 •

edited

Loading