#8323 gsoc19 - Trigger notification task per each artifact creation/modification and add tests

v1.12.0
closed
gsoc19 (11)
General
2019-10-04
2019-08-12
No

Add a test for the send_usermentions_notification task. Also.. send_usermentions_notification task is only called in a few places so far, but would be good to do in all the places where a new artifact is created/modified(content) (new wiki page, new ticket, new blog post, new merge request). Maybe a new ticket for all that together.

Discussion

  • Shalitha Suranga

    Hi.. @brondsem

    Created a new ticket for finalizing new feature and regarding adding missing tests, and support for other artifacts. Note: There are 2 follow up works also

     
  • Shalitha Suranga

    Currently task call is added to ticket, wiki artifacts in ss/8323

     
  • Shalitha Suranga

    Hi.. Added task call to above artifact create/update at ss/8323

    Thanks

     
  • Dave Brondsema

    Dave Brondsema - 2019-08-13

    I didn't notice this earlier, but all send_usermentions_notification usage should be like send_usermentions_notification.post(...) The post is a method automatically added by the @task decorator, and calling post(...) makes it run via the background task queue instead of being run directly as a regular function.

     
    👍
    1
  • Shalitha Suranga

    Hey.. @brondsem

    When I used .post there there is an error from bson

    Here is the error : https://drive.google.com/file/d/1uoAUEUUJJOkRVCA3sL9-KThHSMNVDTgf/view?usp=sharing

    Could you please help to figure out the issue

    Thanks

     

    Last edit: Shalitha Suranga 2019-08-14
    • Dave Brondsema

      Dave Brondsema - 2019-08-14

      Ah yea that makes sense, because post() will create a mongo document representing the task and all its parameters, so the parameters have to be something simple like strings/numbers/lists/etc and not model objects. (The c.user and c.project and c.app models are automatically handled).

      So I would recommend calling send_usermentions_notification with an artifact identifier like .index_id() instead of the full artifact object: send_usermentions_notification.post(ticket.index_id(), ...

      And then load the Artifact instance in the function, something like this I think would work:

      def send_usermentions_notification(artifact_id, ...):
          artifact = ArtifactReference.query.get(_id=artifact_id).artifact
      
       
      👍
      1
  • Shalitha Suranga

    Hi..

    I've updated the task calls and pushed to ss/8323. Btw as you mentioned there needs to be some test coverage So we can add a test to TestNotificationTasks.test_send_usermentions_notification ?

    We can use with mock.patch.object(mail_tasks.smtp_client, '_client') as _client: as in mail tasks I guess. Could you advice your points about test coverage

    Thanks

     
  • Shalitha Suranga

    • status: open --> review
     
  • Dave Brondsema

    Dave Brondsema - 2019-08-18

    Looking good.... adding a test in TestNotificationTasks would be good. I think with sample test data there, there should be a wiki Page artifact created that you could query for. Or make a new artifact of some type. And then use that to call send_usermentions_notification

    And the mock example you gave is good too. That way you can make assertions about what actual mails got sent out.

     
  • Shalitha Suranga

    Hi. @brondsem

    What is the correct way to create wiki instance . I tried project = M.Project.query.get(shortname='test') wiki = project.app_instance('wiki') but wiki is None always :/

    Thanks

     
  • Dave Brondsema

    Dave Brondsema - 2019-08-19

    A common pattern that we use in tests is like this:

    h.set_context('test', 'wiki', neighborhood='Projects')
    

    And then can use c.project and c.app variables just like "normal" non-test code often does. c.app should be the wiki.

    Also since there is such small amount of data in tests by default, you can probably do Page.query.find(...) and not worry about what app it is part of.

    If not of that works and its not finding the project or wiki or page at all, maybe I was wrong about what the setup_basic_test function does in that test. You might need to make a separate test class that also has setup_functional_test in its setUp. That might be the thing that populates some sample data.

     
  • Shalitha Suranga

    Hi..

    I am trying with following test class

    class TestNotificationTasks2(TestController):
        def setUp(self):
            super(TestNotificationTasks2, self).setUp()
            self.setup_with_tools()
    
        @td.with_wiki
        def setup_with_tools(self):
            pass
    
        def test_send_usermentions_notification(self):
            c.user = M.User.by_username('test-admin')
            c.user.set_pref('mention_notifications', True)
            with mock.patch.object(M.User, 'send_user_mention_notification') as um_func:
                d = dict(title='foo', text='new footext')
                r = self.app.post('/wiki/foo/update', params=d)
                print r
    

    Do you think why I am getting this
    302 Found
    The resource was found at http://localhost/wiki/foo/; you should be redirected automatically.

    Thanks

     
    • Dave Brondsema

      Dave Brondsema - 2019-08-25

      The response being a 302 redirect is normal, that's what happens after a form submit, it redirects back to the wiki page. You can do r = r.follow() if you want to continue to the page.

      I wouldn't patch send_user_mention_notification though. We want that to run, and then make assertions about what emails it would send out.

      You probably will need to use M.MonQTask.run_ready() which will make all background tasks run right there in the test. So when send_user_mention_notification.post() is called it'll go into a background task, and then run_ready will make it run. And then you can assert that sendsimplemail was called with the right values (either by patching it, or by checking for its tasks with something like mailtasks = M.MonQTask.query.find(dict(task_name='allura.tasks.mail_tasks.sendsimplemail')).all())

       
      👍
      1
  • Shalitha Suranga

    Hi.. @brondsem

    I have updated ss/8323 with the test case please check

    Thanks

     
  • Dave Brondsema

    Dave Brondsema - 2019-08-27
    • status: review --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2019-08-27

    Looks good, merged! I did notice the link text for merge requests wasn't too great, so created [#8327] for that.

     
    👍
    1

    Related

    Tickets: #8327

  • Dave Brondsema

    Dave Brondsema - 2019-10-04
    • Milestone: unreleased --> v1.12.0
     

Log in to post a comment.