#8023 More flexibility to the site-wide notifications

v1.4.0
closed
Igor Bondarenko
notifications (2) sf-4 (350) 42cc (432)
General
Dave Brondsema
2016-01-11
2015-11-19
Dave Brondsema
No

Site notifications would be more useful if they had options to control who they were shown to and on what pages. For the user, some obvious options are show to everyone, or show to project devs/admins. I can't think of others right now. For the pages, tool type(s) could be a useful selection. But all pages are a tool (plus I'd like this to be able to work with external code that can integrate with Allura) so a url regex would be a good option too. That'd cover lots of possibilities (specific neighborhood, specific project, auth pages, create project page, etc).

Discussion

  • Dave Brondsema

    Dave Brondsema - 2015-11-30
    • labels: notifications --> notifications, sf-current, sf-4
     

  • Igor Bondarenko - 2015-12-01
    • labels: notifications, sf-current, sf-4 --> notifications, sf-current, sf-4, 42cc
    • status: open --> in-progress
    • assigned_to: Igor Bondarenko
     

  • Igor Bondarenko - 2015-12-08

    Closed 867, 868. ib/8023a

     

  • Igor Bondarenko - 2015-12-08
    • status: in-progress --> review
     

  • Dave Brondsema

    Dave Brondsema - 2015-12-09
    • status: review --> in-progress
    • Reviewer: Dave Brondsema
     

  • Dave Brondsema

    Dave Brondsema - 2015-12-09

    I found some ways this can be better, but in general this is working just as it should.

    Performance issue: my_projects_by_role_name returns a mongo query cursor, so calling .count() on it (twice) and .first() will run 3 separate queries. Better to save it to a list first (with .all() or list(...) and then access len(...) and projects[0]. Or, even better probably, would be to change my_projects_by_role_name to run the .all() and return a list for you. Then nobody else using that method will risk the same problem again in the future. It looks like existing uses of my_projects_by_role_name probably won't need to be modified to work with that (maybe a test though).

    Small style note: you don't need is not None most of the time. For example, if note.user_role will work too and is simpler to read.

    In test_get_site_notification_with_page_tool_type_page_regex it'd be better to patch the request.url instead of re.search. Then re.search will run and you can make sure the pattern matching against urls works as expected.

    And instead of request.url I think it'd be more useful to match against request.path_qs which doesn't include the hostname parts. There's no need to match things against the url hostname, and using path_qs will let us write regexes that anchor to the beginning of the path, like ^/u/ to show on any pages in the /u/ neighborhood.

     

    Last edit: Dave Brondsema 2015-12-15

  • Dave Brondsema

    Dave Brondsema - 2015-12-10
    • summary: More flexibility to the site admin notifications --> More flexibility to the site-wide notifications
     

  • Igor Bondarenko - 2015-12-15
    • status: in-progress --> review
     

  • Dave Brondsema

    Dave Brondsema - 2015-12-15
    • status: review --> closed
     

  • Dave Brondsema

    Dave Brondsema - 2016-01-11
    • labels: notifications, sf-current, sf-4, 42cc --> notifications, sf-4, 42cc
     

  • Dave Brondsema

    Dave Brondsema - 2016-04-11
    • Milestone: unreleased --> v1.4.0
     

Log in to post a comment.