#7825 Update "new commits" email template

v1.3.0
closed
General
2015-08-20
2015-02-04
No

See attached screenshot for reference. Several things that can be improved:

  • "From" could be improved, e.g. "{{project name}} {{tool name}} repository"
  • The branch name is specified in front of just the first commit, and then is blank on the rest of the commits. IIRC this is bad for SVN commit messages too since they don't have real branches. I think it could be better moved up as a section header "On master branch:". Test with SVN and with Hg, and with commits on multiple branches.
  • Markdown formatting in commit messages is not expected, e.g. __future__ vs future. When we render commits, e.g. at https://sourceforge.net/p/gazette/code/ci/2cece9731de397e508dfed34d3d6e3866e0a94ac we use a special rendering helper that is configured to do some stuff like artifact linking but not other markdown formatting. We should use that for these commit emails too.
  • It would be cleaner to make the commit message text be a link, instead of showing the full link URL in the email text.
1 Attachments

Related

Tickets: #7896

Discussion

1 2 > >> (Page 1 of 2)
  • Dave Brondsema

    Dave Brondsema - 2015-04-30
    • labels: ux --> ux, sf-current
     
  • Dave Brondsema

    Dave Brondsema - 2015-04-30
    • labels: ux, sf-current --> ux, sf-current, sf-2
     
  • Heith Seewald - 2015-05-05
    • status: open --> in-progress
    • assigned_to: Heith Seewald
     
  • Heith Seewald - 2015-05-27

    Review at: hs/7825

    To QA: Run tests and (ideally) commit to a git/svn/hg repo.

    # from ForgeSVN
    nosetests forgesvn.tests.model.test_repository:TestSVNRev --with-progressive
    

    # from ForgeGit
    nosetests forgegit.tests.model.test_repository:TestGitRepo --with-progressive
    
     
  • Heith Seewald - 2015-05-27
    • status: in-progress --> review
     
  • Heith Seewald - 2015-05-27

    What are your thoughts on: line:398 of repo_refresh.py?

    if isinstance(repo, forgesvn.model.Repository):
        ....
    

    This works well -- but it assumes forgesvn exists.

     
    • Igor Bondarenko - 2015-05-27

      Every Repository model should have tool_name attribute. I think it's better to use that to avoid dependency on forgesvn package.

       
      • Heith Seewald - 2015-05-27

        tool_name is a good idea -- I'll use that instead.

         
  • Igor Bondarenko - 2015-05-27
    • Reviewer: Igor Bondarenko
     
  • Igor Bondarenko - 2015-05-27

    See attachment for reference

    • "From" should be "{{project name}} {{tool name}} repository" as specified in ticket description
    • Link to a user profile is invalid, because of the space. I think it also affects userpic. In fact, name in commit can be different from Allura username (as in this example, I use my full name, so link to /u/Igor Bondarenko is invalid even if spaces will be handled properly).
    • Summary does not use artifact linking ([#1] shoud lilnk to ticket as in commit history view)
    • Branch is specified for each first commit in the branch. I think approach described in ticket description (making branch name header level and then listing commits after). It's really just my preference, I don't sure what would work better.
    • It would also be good if summary where more visible, right now it's kinda lost in other headers and has smaller font. I think it should be easier for the eye to spot summaries instantly within an email.
    • Some tests are failing (see below)

    I didn't check SVN yet.

    ======================================================================
    FAIL: forgegit.tests.functional.test_controllers.TestRootController.test_refresh
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python2.7/site-packages/nose-1.3.4-py2.7.egg/nose/case.py", line 197, in runTest
        self.test(*self.arg)
      File "/home/ibondarenko/ibondarenko-1154/forge/ForgeGit/forgegit/tests/functional/test_controllers.py", line 270, in test_refresh
        assert notification
    AssertionError: 
    
    ======================================================================
    ERROR: test_refresh (forgesvn.tests.model.test_repository.TestRepo)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/ibondarenko/ibondarenko-1154/forge/ForgeSVN/forgesvn/tests/model/test_repository.py", line 878, in test_refresh
        self.repo.refresh()
      File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/model/repository.py", line 650, in refresh
        refresh_repo(self, all_commits, notify, new_clone)
      File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/model/repo_refresh.py", line 158, in refresh_repo
        send_notifications(repo, commit_ids)
      File "/home/ibondarenko/ibondarenko-1154/forge/Allura/allura/model/repo_refresh.py", line 412, in send_notifications
        short_date=ci.authored.date.strftime('%c')
    AttributeError: 'NoneType' object has no attribute 'strftime'
    
     

    Related

    Tickets: #1

  • Igor Bondarenko - 2015-05-27
    • status: review --> in-progress
     
  • Heith Seewald - 2015-05-27

    Great feedback Igor!

    I agree with you on the visual changes and as for the errors... I must have tweaked a few things right before the push (like the strftime for example). Updating now.

     
  • Heith Seewald - 2015-06-02
    • status: in-progress --> review
     
  • Heith Seewald - 2015-06-02

    Because of some of the differences with svn/git, I lost the profile images. But the resulting code is much cleaner.

    hs/7825

     
  • Igor Bondarenko - 2015-06-03
    • status: review --> in-progress
     
  • Igor Bondarenko - 2015-06-03

    Nice work Heith!

    Both code and email layout are definitely cleaner and I like that.

    I'd like to see some future improvements, though:

    • "From:" header now reads as "Repository Test Project git.git". Can we make it something like "Test Project Git repository", instead? I.e "{{project_name}} {{tool_label}} repository"
    • Can we make default branch (app.default_branch_name) appear first in email, even when more than one branch is pushed? It is not essential, but convenient.
    • Commit summary does not use special helpers for markdown formatting. E.g. __future__ converts to bold future, but it should not. We should use g.markdown_commit.convert(commit.message) for that.
    • Apply datetime formatting to commit date. SVN notifications currently show milliseconds "By admin1 on 2015-06-03 09:55:35.169000"
    • Branch headers for mercurial appear before every commit
    • One test fails https://forge-allura.apache.org/p/allura/pastebin/556ecd936d19cd7ef78f888f
     
  • Heith Seewald - 2015-06-04
    • status: in-progress --> review
     
  • Heith Seewald - 2015-06-04

    Great feedback again, thanks Igor :)

    • "From:" header has been fixed.
    • *app.default_branch_name) appear first in email * I spent some time on this one, and had no issues with a git repo, had trouble with mercurial/svn. I'll make a note to revisit it in the future when I'm more familiar with our scm codebase.
    • Fixed the "Commit summary".
    • The commit date is now formatted.
    • Regarding the branch headers for mercurial -- I'm new to mercurial and so it's difficult to distinguish features from bugs in our adapter classes. Basically I'm not exactly sure what is going on here. I could hack it -- but I'd rather understand it deep enough to fix it.
    • Fixed the failing tests.
     
  • Igor Bondarenko - 2015-06-04
    • status: review --> in-progress
     
  • Igor Bondarenko - 2015-06-04

    Good job!

    For sorting branches this might help: Allura/allura/model/repo_refresh.py:_group_commits. It groups commits by branch and by tag, so then you can render all the commits that correspond to default branch first. It should be moved to the utils.py, if we end up using it in some other place.

    It can be hard to understand mercurial branching model after git, been there. I'm not sure my understanding is full also :)

    Branch in mercurial is attribute of every commit. It's not like git when branch is just a pointer to some commit. So, branches=repo.symbolics_for_commit(ci)[0], line always returns branches for hg, whereas for git it returns branches only for the top commit of some branch(es). As a workaround we can keep track of branches of previous commit and if that's equal to current, just return empty list. I guess _group_commits mentioned above might help here as well.

    Could you try this? If this turns out time consuming I'm ok with closing this as is (it is already good enough, especially for git and svn) and creating followup tickets for those issues.

     
  • Heith Seewald - 2015-06-04
    • status: in-progress --> review
     
  • Heith Seewald - 2015-06-04

    That was very helpful, Thanks! Thinking of branches as an attribute in mercurial clears up a lot :)

    I'm still checking into the sorting functionality, but I managed to fix up the mercurial branch headers based on your suggested solution.

     
  • Dave Brondsema

    Dave Brondsema - 2015-06-12

    I've looked at this as well and think it is good as-is. I've created one followup ticket: [#7896]

     

    Related

    Tickets: #7896

  • Dave Brondsema

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

    Dave Brondsema - 2015-06-15
    • labels: ux, sf-current, sf-2 --> ux, sf-2
     
1 2 > >> (Page 1 of 2)

Log in to post a comment.