Added this because we needed the user's information further ahead too

For when a project is deleted, the control first goes to pagure.ui.repo.delete_repo and then to pagure.lib.tasks.delete_project for actually deleting the project asynchronously.

I think we should emit the message from delete_project since the deletion could still fail at that point.

Also, the message should contain the user that deleted the repository (I expect that admins of the upstream repository should be able to delete forks too), which is neither of the arguments passed to delete_project from within delete_repo.

Minor question -- I think that will be accessible using flask.g.fas_user.username, but pagure.lib.tasks does not import flask -- so should I import it there or add an additional argument to delete_project?

Does this approach for project.deleted sound good?

I think we should emit the message from delete_project since the deletion could still fail at that point.

Agreed the fedmsg message should be lower down the stack :)

Using next() here surprised me, I'd have used [0]

so should I import it there or add an additional argument to delete_project?

Additional argument, since it's run asynced and by celery it's out of the scope of a flask session and thus using flask.g won't work.

Does this approach for project.deleted sound good?

yup :)

Let's do one at a time if possible, it'll be easier to review :)

Should we add tests to ensure fedmsg got called properly? We should be able to mock it and check it got called and somewhat check the arguments it got passed :)

Why this change? Not really seeing the benefit since in the first case you stop once you encounter the target while in the second case you iterate through the entire list.

Looks good (except for one change).

Note that this PR is now conflicting, so needs a rebase :)

Also, we'll need to add support for these messages on fedmsg-meta-fedora-infrastructure before we can push this code in prod.

Okay, I made that change and rebased the commits.

But the test for project.deleted is failing due to the mock for pagure.lib.notify.log not capturing the call (possibly because of it being emitted asynchronously?).

Sorry for the redundant messages ^^^, I was just arranging changes in the right commits and doing force pushes (since they're still in my fork)

I've tested it locally and it seems to no work:

DetachedInstanceError: Parent instance <Project at 0x7f482bfaff10> is not bound to a Session; lazy load operation of attribute 'admins' cannot proceed

Which makes sense since the project is deleted before trying to access it to send the fedmsg message.

This is the change I'd like to add to this PR to make it work:

diff --git a/ pagure/lib/notify.py b/ pagure/lib/notify.py
index 625c848..c054e79 100644
--- a/ pagure/lib/notify.py     
+++ b/ pagure/lib/notify.py     
@@ -46,6 +46,7 @@ def fedmsg_publish(*args, **kwargs):  # pragma: no cover
     # pylint: disable=import-error
     kwargs['modname'] = 'pagure'
     kwargs['cert_prefix'] = 'pagure'
+    kwargs['active'] = True
     try:
         import fedmsg
         fedmsg.publish(*args, **kwargs)
diff --git a/ pagure/lib/tasks.py b/ pagure/lib/tasks.py
index 20ae2a5..337c6a1 100644
--- a/ pagure/lib/tasks.py      
+++ b/ pagure/lib/tasks.py      
@@ -185,13 +185,14 @@ def delete_project(namespace=None, name=None, user=None, action_user=None):
     # Remove the project from the DB
     username = project.user.user
     try:
+        project_json = project.to_json(public=True)
         session.delete(project)
         session.commit()
         pagure.lib.notify.log(
             project,
             topic='project.deleted',
             msg=dict(
-                project=project.to_json(public=True),
+                project=project_json,
                 agent=action_user,
             ),
         )

Diff reviewed by @puiterwijk , going to amend and merge the PR locally.

Thanks! :)

Commit 81865860 fixes this pull-request