Prehook user creation #378

ChaamC · 2020-12-17T16:49:01Z

This PR adds the webhook feature for user creation/deletion. We add the desired urls to the config.yml file.

Note that there is currently no error handling if the webhook fails, since it is not in Magpie's responsabilities.
Magpie only sends the webhook requests and continues with its current jobs. The webhook requests are non blocking.

…ehook-user_creation

dbyrns

Great work. Even if temp url is not available right now, I think that we can do more stuff right now to be better prepared.

env/magpie.env.example

config/magpie.ini

magpie/api/management/user/user_utils.py

docs/configuration.rst

magpie/api/management/user/user_formats.py

magpie/api/management/user/user_utils.py

tests/test_magpie_api.py

config/config.yml

docs/configuration.rst

env/magpie.env.example

magpie/api/management/user/user_formats.py

magpie/api/management/user/user_utils.py

magpie/api/management/user/user_views.py

fmigneault · 2020-12-18T19:38:16Z

tests/utils.py

+    x.start()
+
+    return webhook_app_instance
+


All the webhook_app and handling of serve/threading can be simplified with TestApp using same process as in get_test_magpie_app.
Doesn't need extra waitress and all other util functions will be available directly.

@fmigneault
The problem I had for creating the webhook app is that the context was a bit different.
For the Magpie app in the tests, we create a TestApp, and we then access it directly to execute the multiple requests required.
For the webhook app, it has to be accessible via an URL from inside the Magpie app, so I couldn't use the TestApp approach, since that app would not be accessible where the webhooks requests would be called. I had to make it available via an URL, using waitress.

fmigneault · 2020-12-18T19:38:44Z

requirements-dev.txt

@@ -18,3 +18,4 @@ pyramid-twitcher==0.5.3
 pytest
 tox>=3.0
 webtest
+waitress


extra dependency not needed, see other comment

tests/test_magpie_api.py

docs/configuration.rst

fmigneault · 2020-12-18T20:04:08Z

tests/test_magpie_api.py

+                # Check if user deletion was successful even if the webhook failed
+                sleep(1)
+                utils.check_response_basic_info(resp, 200, expected_method="GET")
+                utils.TestSetup.check_NonExistingTestUser(self)


There is nothing that seems to validate that Magpie sent the request to the webhook URL.
Should have some kind of mock of the error callback to ensure it was called.

fmigneault · 2020-12-18T20:04:51Z

tests/test_magpie_api.py

+                # Check if user deletion was successful even if no webhooks were defined in the config
+                sleep(1)
+                utils.check_response_basic_info(resp, 200, expected_method="GET")
+                utils.TestSetup.check_NonExistingTestUser(self)


idem as previous with validation mock

fmigneault · 2020-12-19T01:02:48Z

@ChaamC
Next time you work on this, please add a commit with (fixes #343) in the message.
Also add the same formatted issue URL in CHANGES.rst to keep the trace.

…rwritten

ChaamC · 2021-01-13T16:20:10Z

Version #2 of the PR🙂
@fmigneault @dbyrns

Main changes are :

Change the webhook config
- Add template parameters in the payload
Refactoring :
- move webhook tests and other webhook functions to distinct files
Check the webhook config at the beginning, when creating the magpie app
Change user status in case of an error with the webhooks

I did not change the app (the point about using TestApp instead of waitress), see the related response :

@fmigneault
The problem I had for creating the webhook app is that the context was a bit different.
For the Magpie app in the tests, we create a TestApp, and we then access it directly to execute the multiple requests required.
For the webhook app, it has to be accessible via an URL from inside the Magpie app, so I couldn't use the TestApp approach, since that app would not be accessible where the webhooks requests would be called. I had to make it available via an URL, using waitress.

About the mocking aspect, I didn't use the suggested mock. I wasn't sure in what case to use it, but let me know if the new tests are okay.
For the create_user case, I validate if Magpie sent the webhook, either by checking the webhook_status (in case of success), or checking the user's status (it should be 0 in case of a fail.)
For the delete_user case, we established a webhook fail should not prevent user deletion, and we do not have to process the error cases. I still check the webhook_status in case of success here.

I will still have to merge Francis' new code after this and finish the part about the tmp_url. I will do another PR after this.

ChaamC · 2021-01-18T17:22:50Z

@fmigneault
Codacy seems to complain about one new issue in the webhooks.py file :
Multi-line docstring summary should start at the first line (D212)

There seems to already have other issues in other files, so do I just ignore this one too?
I couldn't fix it, since starting the docstring at the start of the first line brought up an error in check-docf. Probably because docf complains about the docstring starting on the first line... I think they are conflicting issues.

dbyrns

LGTM although I suggest a snippet to simplify the replace_template function.

magpie/api/webhooks.py

dbyrns

I got a new small request for you.

magpie/api/webhooks.py

dbyrns

Ok I'm super happy with it now :)

…ehook-user_creation

fmigneault

Should be good to go. Waiting for PR #383 with extra code on top of this one.
Fixes #343

@ChaamC
You can either change the base of #383 to be master, so that it includes both PRs, or merge it first and then this one after in master, as you prefer. Just make sure the whole test suites runs on both combined.

Finish the missing tmp_url part for the webhooks

…ehook-user_creation

codecov-io · 2021-02-09T20:48:46Z

Codecov Report

Merging #378 (4dc4e0c) into master (00769ce) will increase coverage by 0.20%.
The diff coverage is 72.44%.

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
+ Coverage   75.93%   76.13%   +0.20%     
==========================================
  Files          65       66       +1     
  Lines        7882     7974      +92     
  Branches     1097     1118      +21     
==========================================
+ Hits         5985     6071      +86     
- Misses       1621     1622       +1     
- Partials      276      281       +5

Impacted Files	Coverage Δ
magpie/api/management/user/user_formats.py	`100.00% <ø> (ø)`
magpie/register.py	`44.32% <ø> (+0.51%)`	⬆️
magpie/api/management/register/register_utils.py	`61.90% <57.14%> (+7.85%)`	⬆️
magpie/api/webhooks.py	`61.70% <61.70%> (ø)`
magpie/app.py	`84.50% <71.42%> (-5.69%)`	⬇️
magpie/__meta__.py	`100.00% <100.00%> (ø)`
magpie/api/management/user/user_utils.py	`94.19% <100.00%> (+0.33%)`	⬆️
magpie/api/management/user/user_views.py	`98.15% <100.00%> (+0.01%)`	⬆️
magpie/api/schemas.py	`99.46% <100.00%> (+<0.01%)`	⬆️
magpie/models.py	`93.42% <100.00%> (+2.63%)`	⬆️
... and 6 more

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 1dc3e4d...4dc4e0c. Read the comment docs.

ChaamC added 6 commits December 10, 2020 08:22

base webhook integration

f7c1a52

change webhook feature and add all webhook tests

fa0ab13

updated docs with webhooks info

66fc1c1

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into pr…

927ad29

…ehook-user_creation

fix linter

af5b257

fix doc

d393703

ChaamC requested review from dbyrns and fmigneault December 17, 2020 16:49

github-actions bot added api Something related to the API operations doc Documentation improvements or building problem tests Test execution or additional use cases labels Dec 17, 2020

dbyrns suggested changes Dec 17, 2020

View reviewed changes

fmigneault requested changes Dec 18, 2020

View reviewed changes

fmigneault reviewed Dec 18, 2020

View reviewed changes

docs/configuration.rst Show resolved Hide resolved

fmigneault reviewed Dec 18, 2020

View reviewed changes

ChaamC added 3 commits January 4, 2021 10:59

move webhook operations

bc7b78a

remove webhook env variable, and fix broken tests if env variable ove…

39dcb2e

…rwritten

move webhooks tests

c704e5d

fmigneault mentioned this pull request Jan 6, 2021

tmp-url #379

Merged

ChaamC added 8 commits January 7, 2021 14:29

webhook tests refactoring and checking webhook config

8a59b17

change user status on error, test for non existent url

18ed180

move webhooks code

a50fbf5

refactor webhook code

0099a0d

updated doc

d5d0d1a

fix webhook config and complete doc (#343)

b1692c3

fix lint

b058990

fix docf lint

dc69fc9

fix lint

b3e03b5

ChaamC requested a review from dbyrns January 18, 2021 17:23

dbyrns approved these changes Jan 18, 2021

View reviewed changes

magpie/api/webhooks.py Outdated Show resolved Hide resolved

simplified dict template replacing

ce02667

dbyrns suggested changes Jan 21, 2021

View reviewed changes

magpie/api/webhooks.py Outdated Show resolved Hide resolved

magpie/api/webhooks.py Outdated Show resolved Hide resolved

ChaamC added 2 commits January 22, 2021 16:24

add url templating and reoganize replace_template function

5c8eb69

optimize template webhook request

c31b9ae

dbyrns approved these changes Jan 25, 2021

View reviewed changes

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into pr…

9283bfe

…ehook-user_creation

github-actions bot added the ci Something related to code tests, deployment and packaging label Jan 25, 2021

ChaamC added 5 commits January 26, 2021 10:59

fix fstrings

2190641

implement webhook_error case for the tmp_url

9445253

change webhook action enum

3d0a2b4

small fixes after pr update

dd44855

fix lint

2cd96ec

fmigneault approved these changes Feb 3, 2021

View reviewed changes

Charles-William Cummings and others added 2 commits February 8, 2021 10:11

fix webhook action names

1d801d0

Merge pull request #383 from Ouranosinc/webhook_tmp_url

a390dc1

Finish the missing tmp_url part for the webhooks

github-actions bot added the db Issues related to database connection, migration or data models label Feb 8, 2021

Charles-William Cummings and others added 2 commits February 8, 2021 10:45

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into pr…

75462c5

…ehook-user_creation

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into pr…

957718c

…ehook-user_creation

cwcummings added 2 commits February 9, 2021 16:01

fix test admin user

f012170

fix for py3.5 test

20c4380

ChaamC force-pushed the prehook-user_creation branch from 4dc4e0c to 20c4380 Compare February 9, 2021 21:13

fix lint

89209fc

ChaamC merged commit e154548 into master Feb 9, 2021

ChaamC deleted the prehook-user_creation branch February 9, 2021 21:32

fmigneault mentioned this pull request Apr 8, 2021

[Feature] Webhook URL for user creation #343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prehook user creation #378

Prehook user creation #378

ChaamC commented Dec 17, 2020

dbyrns left a comment

fmigneault Dec 18, 2020

ChaamC Jan 5, 2021

fmigneault Dec 18, 2020

fmigneault Dec 18, 2020

fmigneault Dec 18, 2020

fmigneault commented Dec 19, 2020

ChaamC commented Jan 13, 2021

ChaamC commented Jan 18, 2021

dbyrns left a comment

dbyrns left a comment

dbyrns left a comment

fmigneault left a comment •

edited

Loading

codecov-io commented Feb 9, 2021

Prehook user creation #378

Prehook user creation #378

Conversation

ChaamC commented Dec 17, 2020

dbyrns left a comment

Choose a reason for hiding this comment

fmigneault Dec 18, 2020

Choose a reason for hiding this comment

ChaamC Jan 5, 2021

Choose a reason for hiding this comment

fmigneault Dec 18, 2020

Choose a reason for hiding this comment

fmigneault Dec 18, 2020

Choose a reason for hiding this comment

fmigneault Dec 18, 2020

Choose a reason for hiding this comment

fmigneault commented Dec 19, 2020

ChaamC commented Jan 13, 2021

ChaamC commented Jan 18, 2021

dbyrns left a comment

Choose a reason for hiding this comment

dbyrns left a comment

Choose a reason for hiding this comment

dbyrns left a comment

Choose a reason for hiding this comment

fmigneault left a comment • edited Loading

Choose a reason for hiding this comment

codecov-io commented Feb 9, 2021

Codecov Report

fmigneault left a comment •

edited

Loading