Apache Allura™

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

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.

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'

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.

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

hs/7825

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

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.

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.

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.

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