Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove type from call and consider type on path for helloapp #99

Merged
merged 4 commits into from
Mar 17, 2021

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Mar 11, 2021

Summary

Remove type from call and consider type on path for helloapp

Ticket Link

https://mattermost.atlassian.net/browse/MM-33178

Related Pull Requests

Redux: mattermost/mattermost-redux#1406
Webapp: mattermost/mattermost-webapp#7679
Mobile: mattermost/mattermost-mobile#5221

@@ -19,9 +20,6 @@ func (h *helloapp) GetBindings(w http.ResponseWriter, req *http.Request, claims
}

func (h *helloapp) Install(w http.ResponseWriter, req *http.Request, claims *api.JWTClaims, c *apps.CallRequest) (int, error) {
if c.Type != apps.CallTypeSubmit {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to remove this? Has anything changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since calls don't have type anymore, there is no need to check the type here. Since install is called from the server, it does not go under a subpath (e.g. /install/submit) but directly to the endpoint.

Does it make sense? Or should all calls (including bindings and install) go to the subpath?

@levb thoughts?

@mickmister
Copy link
Contributor

/update-branch

@codecov-io
Copy link

codecov-io commented Mar 13, 2021

Codecov Report

Merging #99 (b9bcf61) into master (455bd28) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   24.48%   24.48%           
=======================================
  Files          27       27           
  Lines         976      976           
=======================================
  Hits          239      239           
  Misses        721      721           
  Partials       16       16           

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 455bd28...b9bcf61. Read the comment docs.

@larkox larkox merged commit 71763bd into mattermost:master Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants