Add example Apps in additional languages - Python #352

dsharma522 · 2022-07-22T18:39:23Z

Summary

Ticket Link

Github Issue - #351

This PR adds basic example of Hello-world app in Python language. It also includes the required requirements.txt file which will be needed.

mattermod · 2022-07-22T18:39:26Z

Hello @dsharma522,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

dsharma522 · 2022-07-22T18:42:53Z

/check-cla

mickmister

Thanks for this contribution @dsharma522 🎉 LGTM, though I have a request to make the PORT and ROOT_URL configurable via environment variables.

mickmister · 2022-07-22T18:56:07Z

examples/python/hello-world.py

+APP_PORT = 8080
+APP_HOST = 'localhost'
+ROOT_URL = 'http://localhost:8080'


Can this be made configurable using environment variables? The port and root URL specifically should be configurable. Please verify it works with an ngrok URL.

Noted! will make it configurable and push commit.

codecov-commenter · 2022-07-23T07:01:25Z

Codecov Report

Merging #352 (d2bc7b7) into master (dbc7a94) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #352   +/-   ##
=======================================
  Coverage   27.09%   27.09%           
=======================================
  Files          88       88           
  Lines        5155     5155           
=======================================
  Hits         1397     1397           
  Misses       3608     3608           
  Partials      150      150

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mickmister · 2022-07-23T17:31:44Z

examples/python/README.md

+- have python installed , preferably `>=3.0`
+- install the requirements mentioned in the `requirement.txt`
+- make required changes in the `settings.json` to run the app on custom url & port


Suggested change

- have python installed , preferably `>=3.0`

- install the requirements mentioned in the `requirement.txt`

- make required changes in the `settings.json` to run the app on custom url & port

- Have python installed, preferably `>=3.0`

- Install the requirements mentioned in the `requirement.txt`

- Make required changes in the `settings.json` to run the app on custom url & port

mickmister · 2022-07-23T17:34:22Z

examples/python/settings.json

+{
+    "port": "8080",
+    "root_url": "https://6fe6-103-157-160-168.in.ngrok.io",
+    "host": "0.0.0.0"
+}


I think it's confusing how there are 3 sources of configuration (hardcode, settings file, and env vars). I'm thinking we should get rid of this file, as it's not something established among the examples already

mickmister · 2022-07-23T17:35:08Z

examples/python/src/hello-world.py

+    root_dir = os.path.dirname(os.path.dirname(__file__))
+    with open(f'{root_dir}/settings.json') as config_file:
+        settings = json.load(config_file)
+        for env_name, env_value in settings.items():
+            os.environ[env_name] = env_value


The environment variables should take precedence over the settings.json file, rather than the settings.json file editing the environment here. At the moment, the environment variables will not do anything unless we delete the settings.json file.

Removed settings.json file and will use the configured environment variables. The app will use default ones in case not configured.

mickmister · 2022-07-23T17:36:20Z

examples/python/src/hello-world.py

+def _subscribe_team_join(context: dict) -> None:
+    site_url = context['mattermost_site_url']
+    bot_access_token = context['bot_access_token']
+    url = site_url + 'plugins/com.mattermost.apps/api/v1/subscribe'


Should we do path.join here instead? Does the site URL always end in a slash?

mickmister · 2022-07-23T17:39:16Z

examples/python/README.md

@@ -0,0 +1,7 @@
+### Pre-requisite
+- have python installed , preferably `>=3.0`
+- install the requirements mentioned in the `requirement.txt`


pip3 install -r requirements.txt right? We should explain that here

mickmister · 2022-07-23T17:42:22Z

examples/python/src/hello-world.py

+                            'expand': {
+                                'app': 'all',
+                                # if you want to expand team & channel, ensure that bot is added to the team & channel
+                                # else command will fail to expand the context
+                                # 'team': 'all',
+                                # 'channel': 'all',
+                            },


if you want to expand team & channel, ensure that bot is added to the team & channel
else command will fail to expand the context

The bot must be a member of the team and channel, otherwise the expand fails and the request is never sent to the App? It seems that we shouldn't show the command to the user in this case. cc @levb

@levb @mickmister ,

I am able to get the app in the context without even adding the bot to the team/channel. And as expected expansion of team , channel failed as it is not the part of any team/channel currently.

Steps to reproduce

install the app via /apps install http <url> (use the same app binding as in the current PR)

run the command /first-command and press enter to submit. The command will succeed and won't show any error.

Sharing raw response from ngrok for ref, if needed.

{ "path": "/first_command", "expand": { "app": "all" }, "context": { "app_id": "hello-world", "location": "/command/first-command", "user_agent": "webapp", "track_as_submit": true, "mattermost_site_url": "http://localhost:8066", "developer_mode": true, "app_path": "/plugins/com.mattermost.apps/apps/hello-world", "bot_user_id": "3xxj6snbwby19npr1tca3ys9zo", "bot_access_token": "3mf1popcwjnzxyeps9kqsqeuoc", "app": { "app_id": "hello-world", "webhook_secret": "zrp5e37iejgtixpfqtjt43ibua", "bot_user_id": "3xxj6snbwby19npr1tca3ys9zo", "bot_username": "hello-world", "remote_oauth2": {} }, "acting_user": { "id": "kredsjwqwbrp9r41c86b69akno", "delete_at": 0, "username": "", "auth_service": "", "email": "", "nickname": "", "first_name": "", "last_name": "", "position": "", "roles": "", "locale": "", "timezone": null, "disable_welcome_email": false }, "oauth2": {} }, "raw_command": "/first-command " }

Okay so when you say "command will fail to expand the context", it's sort of a silent error, and the app still receives the request. It's just not the full request with all values. I think this is due to new changes to expand (optional vs required), depending on which version of the App framework you're working with.

No actually "command will fail to expand the context" mean is that command will fail in the UI itself and not silent failure. And app won't receive the request.

mickmister · 2022-07-23T17:43:54Z

examples/python/src/hello-world.py

+                    {
+                        'description': 'test command',
+                        'hint': '[This is testing command]',
+                        # this will be the command displayed to user as /second-command
+                        'label': 'second-command',
+                        'icon': 'icon.png',


This should instead be in the same array that begins on line 73, rather than creating the new object on line 95

mickmister · 2022-07-23T17:46:45Z

examples/python/src/hello-world.py

+    # Here one can subscribe to channel_joined/left events as these required team_id now to be subscribed,
+    # hence use the team_id received in the event and make a call for subscribing to channel_joined/left events.


The framework should make sure these subscriptions are removed when the bot is removed from this team cc @levb for discussion

mickmister · 2022-07-25T13:46:15Z

examples/python/README.md

+  export APP_PORT=8080
+  export ROOT_URL=http://localhost:8080
+  export APP_HOST=localhost


Some PaaS systems require you to use PORT as the env variable for the port to listen on. Let's stick with PORT

mickmister · 2022-07-25T13:49:11Z

examples/python/src/hello-world.py

+app = Flask(__name__, static_url_path='/static', static_folder='./static')
+
+DEFAULT_APP_PORT = 8080
+DEFAULT_APP_HOST = 'localhost'


For ease of configurability, maybe we make this 0.0.0.0? Also let's make the env var be HOST

mickmister · 2022-07-25T13:49:49Z

examples/python/src/hello-world.py

+DEFAULT_APP_PORT = 8080
+DEFAULT_APP_HOST = 'localhost'
+DEFAULT_ROOT_URL = 'http://localhost:8080'
+FORM = {


Not sure why this is capitalized. Even though it's only defined once, it's not really a "constant"

replaced with lower case variable names

mickmister · 2022-07-25T13:52:22Z

examples/python/src/hello-world.py

+                            'expand': {
+                                'app': 'all',
+                                # if you want to expand team & channel, ensure that bot is added to the team & channel
+                                # else command will fail to expand the context
+                                # 'team': 'all',
+                                # 'channel': 'all',
+                            },


Okay so when you say "command will fail to expand the context", it's sort of a silent error, and the app still receives the request. It's just not the full request with all values. I think this is due to new changes to expand (optional vs required), depending on which version of the App framework you're working with.

mattermod · 2022-08-05T01:01:17Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mickmister

LGTM! Just a few more suggestions

mickmister · 2022-08-12T16:38:25Z

examples/python/src/hello-world.py

+                        'bindings': [
+                            {
+                                'location': 'send',
+                                'label': 'send',
+                                'form': form
+                            },


Can we have a comment explaining that this is how you make a subcommand /second-command send? Mainly because there are already plenty of comments explaining things around this area, and this is an important concept to understand imo. Also maybe a comment mentioning that we are including the form here

Also I know I said that form shouldn't be capitalized, but maybe to make it stand out more, we can make it say SHARED_FORM

neflyte · 2022-08-15T12:43:57Z

Would it be possible to improve the instructions so they include step-by-step guides for getting started? For example, link to python.org for install instructions, describe (or link to) how to install the dependencies in requirements.txt (or some other dependency manager like pipenv), what changes are necessary in settings.json.

It would be best to include this information now, before merging, so it isn't missed in the future.

hanzei · 2023-01-31T11:15:57Z

Documented in https://developers.mattermost.com/integrate/apps/quickstart/quick-start-python/

Add example Apps in additional languages - Python

3b75b06

mickmister requested review from mickmister, DHaussermann and levb July 22, 2022 18:54

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 22, 2022

mickmister suggested changes Jul 22, 2022

View reviewed changes

dsharma522 added 2 commits July 23, 2022 12:12

more examples + configurable environments

0be28a1

minor change

d73cd8a

dsharma522 requested a review from mickmister July 23, 2022 07:04

removed extra file

28e2b62

mickmister suggested changes Jul 23, 2022

View reviewed changes

comments

3732dd0

dsharma522 requested a review from mickmister July 23, 2022 19:35

mickmister reviewed Jul 25, 2022

View reviewed changes

comments

8a54962

dsharma522 requested a review from mickmister July 25, 2022 17:19

mattermod added the Lifecycle/1:stale label Aug 5, 2022

mickmister approved these changes Aug 12, 2022

View reviewed changes

comments

6a7efeb

mickmister approved these changes Aug 22, 2022

View reviewed changes

Merge branch 'master' into master

d2bc7b7

dsharma522 closed this by deleting the head repository Aug 22, 2022

dsharma522 reopened this Aug 22, 2022

mickmister merged commit 5d18631 into mattermost:master Aug 22, 2022

mickmister mentioned this pull request Aug 22, 2022

Request to community: Please submit examples of your Apps 🚀 #351

Open

This was referenced Aug 24, 2022

Expand app quick start guides mattermost/mattermost-developer-documentation#1106

Merged

Move /examples and /dev to new mattermost-app-examples repository #357

Closed

azigler mentioned this pull request Aug 31, 2022

Move /examples and /dev to new mattermost-app-examples repository #362

Merged

hanzei added Docs/Done Required documentation has been written and removed Lifecycle/1:stale labels Jan 31, 2023

hanzei added this to the v1.2.0 milestone Jan 31, 2023

		# Here one can subscribe to channel_joined/left events as these required team_id now to be subscribed,
		# hence use the team_id received in the event and make a call for subscribing to channel_joined/left events.

Add example Apps in additional languages - Python #352

Add example Apps in additional languages - Python #352

Conversation

dsharma522 commented Jul 22, 2022

Summary

Ticket Link

mattermod commented Jul 22, 2022

dsharma522 commented Jul 22, 2022 • edited Loading

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 23, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Jul 23, 2022 • edited Loading

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

dsharma522 Jul 23, 2022 • edited Loading

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

mattermod commented Aug 5, 2022

mickmister 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

neflyte commented Aug 15, 2022 • edited Loading

hanzei commented Jan 31, 2023

dsharma522 commented Jul 22, 2022 •

edited

Loading

codecov-commenter commented Jul 23, 2022 •

edited

Loading

mickmister Jul 23, 2022 •

edited

Loading

dsharma522 Jul 23, 2022 •

edited

Loading

neflyte commented Aug 15, 2022 •

edited

Loading