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

Webhooks #2624

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Webhooks #2624

wants to merge 16 commits into from

Conversation

abulte
Copy link
Contributor

@abulte abulte commented Jun 17, 2021

A naive approach at dispatching webhooks.

For now, only the dispatch logic is here, we still need to plug it to signals (dataset created...).

Plugged-in signals:

  • datagouvfr.dataset.created
  • datagouvfr.dataset.updated
  • datagouvfr.dataset.deleted
  • datagouvfr.discussion.created
  • datagouvfr.discussion.closed
  • datagouvfr.discussion.commented
  • datagouvfr.organization.created
  • datagouvfr.organization.updated
  • datagouvfr.reuse.created
  • datagouvfr.reuse.updated
  • datagouvfr.community_resource.created
  • datagouvfr.community_resource.updated

Also, some linting changes: enforce single quotes through flake8, lint legacy files as I go through them.

This is designed to work with https://github.com/abulte/chatelet, but it's pretty generic in the end: send a POST request to a designated URL.

@abulte
Copy link
Contributor Author

abulte commented Jun 19, 2021

@quaxsze some hell broke loose in the tests because

@blueprint.route('/<user:user>/', endpoint='show')
disappeared (errors were hidden before but my modifications made them pop), we should discuss this.

@abulte abulte requested review from quaxsze and maudetes June 19, 2021 18:24
@abulte abulte marked this pull request as ready for review June 19, 2021 18:31
@maudetes
Copy link
Contributor

How did you pick the signals to plug in ? For example, don't we want to add the signals for resources addition and deletion (Resource.on_added, Resource.on_deleted that don't look connected at all today) ?



@Dataset.on_delete.connect
def on_dataset_delete(dataset):
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to add the if not dataset.private condition on dataset deletion ?

updates, _ = dataset._delta()
if 'private' in updates and not dataset.private:
dispatch('datagouvfr.dataset.created', dataset.to_json())
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by the logic on dataset privacy here. If an update is made on a private dataset, we're going to dispatch the updated signal?

assert True

def test_webhooks_task(self, rmock):
'''NB: apparently celery task errors don't surface so we need to test them directly'''
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this NB here, as we don't test task errors in this particular test? Maybe move it somewhere else?



@on_new_discussion.connect
def on_new_discussion(discussion):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question on discussion events, don't we want to add the if not dataset.private condition on dataset deletion ?

Base automatically changed from remove-theme to master July 6, 2021 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants