Apache Allura™

• qa: Tim Van Steenburgh

• status: open --> code-review

• status: code-review --> in-progress

Looking good! Here are a few initial thoughts:

1. Looks like some .svn directories got committed - those need to be removed.
2. I see you are iterating over entry points in a number of places. I wonder if that code might be consolidated, or the entry points cached, perhaps like we do in app_globals.py
3. Every time we create an event, we loop over g.statslisteners. I wonder if the loop could be moved into a method or function so that the rest of the code could just call postEvent(...) (or similar).
4. I see you've started to add some tests, which is great. I would like to see even more test coverage, especially unit tests covering the model.
5. Finally, I see that stats is meant to be pluggable in some way, as we are using entry points. I must admit, however, that I don't fully understand how one would plug in new stats functionality. Can you elaborate on your vision for the "pluggability" of stats?

This is by no means a comprehensive review, but rather some of my first impressions. Sorry it took a while. In the future, committing smaller chunks of code will make the review process go faster.

Thanks again for this contribution. I'm looking forward to seeing it come together. Please feel free to continue discussion either on the ticket itself, or on the mailing list.

Thank you for your feedback!
I removed .svn directories (I'm sorry, I didn't notice they were there) and I cached entry points in app_globals.py, as you suggested.
In order to avoid writing the loop every time an event is called, I moved it creating a new class to do so, in eventslistener.py.
I also added unit tests to cover functionalities of the model.
For what concerns pluggability, our idea is to allow registering a new listener by simply creating a new entry point with allura.stats as its group. The entry point should be built in a way similar to the one defined for UserStats: a class, in our case ForgeUserStatsApp, should be implemented, including an attribute listener representing the listener class which implements the logic needed to update the statistics. The listener should be a subclass of EventsListener, defined in eventslistener.py.
At the moment, there is no loop through all the allura.stats entry points within app_globals.py to load all the entry points, but we only load the listener we created. However, it is very simple to put back the loop, so that this mechanism can actually be used. At the moment, we also have values in the configuration file to enable or disable user stats, so that it is also possible to disable it even if the module is actually installed within the forge. The loop would simply be:

statslisteners = []
for name, ep in self.entry_points['stats'].iteritems():
    if name == 'userstats':
        if self.show_userstats:
            statslisteners.append(ep().listener)
    else:
        statslisteners.append(ep().listener)

Let us know what you think about this mechanism, and if a more detailed explanation is needed. Moreover, feel free to add other comments about our code.
Finally, please note that, even if all the code has been submitted under my name, I actually developed it together with my fellow student, Simone Gatti.

• milestone: limbo --> forge-backlog

Tim recently rebased the branch so I could look at it easily too. I scanned through the net difference by running git diff origin/master... so I didn't see all the individual commits :)

• There are a few changes that seem unrelated: the upload_sshkey method and one line in Allura/allura/controllers/discuss.py Are those related? Or fixing other bugs?
• many of the methods on Stats do list(self.query.find()) and then go through everything. We'll have to keep our eye on performance for those. Not sure it'll be an issue for sure, but seems like that could get slow once this is running on a big site like SourceForge :)
• The name contrib_stats.py seems a bit odd to me. "contrib" doesn't seem necessary. And if it's only used for User stats, maybe it could go into that tool. On the other hand, if it is generic and could be use for project stats some day, then keeping it in the Allura package makes sense too. The Allura/allura/model/stats.py file is very old and as far as I can tell it's not used (used to be for some "content production activity" stats which are no longer part of allura). Feel free to delete it and rename yours to stats.py
• Allura/allura/templates/user_preferences.html got split up in [#5909] so on the si/5453 branch we need to delete that file, and copy any changes over into the new files
• now that this is a tool, should we remove the "User statistics" little section from the user_index.html file?
• can you remove the .sample files from the testgit repo? they aren't needed and the pre-rebase.sample file has a copyright headers so we want to avoid that.
• registration_date gets set to the date that the UserStats object was created. When I saw registration date on the stats page, I though about when the user's account was registered. For most Allura instances, you can use user._id.generation_time but some instances (like SourceForge) need to hook up to older user systems, which happens via the AuthenticationProvider plugin. Could you add a def user_registration_date(self, user): method and implement it with user._id.generation_time for LocalAuthenticationProvider? Then we at SF can implement that method in our own provider too.

Sorry for all this extra feedback. I know Tim's given you a lot already and it's getting really close. That's just the reality of big new features :)

Thanks

numpy & matplotlib are some pretty big dependencies to add (large, and require C module compilation). The graphs so far are pretty simple, so we might want to do them just in CSS instead. There's even good JS/CSS libraries for complex visualizations. Not something we necessarily need to change before merging this, but wanted to mention my preferred direction on it. Maybe we can change it and remove numpy/matploglib down the road.

Hi Dave!

• The line we changed in discuss.py is needed to correctly update statistics about artifacts.
• The upload_sshkey method is not something we have intentionally added. Either we did it for mistake, or it was somehow added as a consequence of some merge, since at the beginning of the history of this branch we didn't have the SubscriptionsController, but it was added during the merge. Anyway, we removed this method.
• We changed the code of Stats so that it no longer uses list(). We still have to iterate through all the elements, but this should improve performances.
• We didn't know that the old stats.py was useless. Now we removed it and replaced with our file. We decided to put it there because we also used the same class for our tool to gather statistics about organizations, which we have already implemented and we plan to upload in the branch including the concept of organizations as soon as possible. Moreover, it would be possible, for example, to use the same class for statistics about a single project, as you said. Anyway, as a consequence of replacing the old stats.py file, we had to change something else. We hope everything is ok. If something went wrong, feel free to put it back or to give us hints about how to change this.
• The link to user stats in the user profile has been removed now.
• The registration date is now set as you suggested. We didn't know about this option, thank you for this suggestion!
• The .sample files have been removed.
• For what concerns graphs, we thought someone could add more complex ones in the future, therefore we decided to use mathplotlib. However, as you said, in the future we might consider removing it and using JS/CSS instead.
• Finally, for what concerns user_preferences.html, we removed the file and everything looks to be ok.

All your changes here are looking good.

I've got a few semi-related changes that I'm working on. I'll push them tomorrow probably and let you review them if you want.

I've pushed a few commits to db/5453 Do you want to review those?

Yes, I was very happy to review your commits. Thank you, I think it is a very nice improvement!
It looks very good to me, and I don't have anything to add.

I think we are ready to push our commits on the master branch! Do you agree?

Yes, I just wanted to incorporate some recent changes Tim made that will allow us to not count all the commits that are processed during an SVN import.

• status: in-progress --> closed

Merged to master

• milestone: forge-backlog --> forge-apr-05

Originally by: *anonymous

This is the error i am getting while running paster setup-app development.ini

INFO [allura.model.notification] Notifications disabled for project u/test-user, not sending metadata(<page import_id="None" labels="I[]" deleted="False" text="u'Welcome" to="" your="" wiki!\n\nthis="" is="" the="" default="" page,="" edit="" it="" as="" you="" see="" fit.="" add="" a="" new="" page="" simply="" reference="" within="" brackets,="" e.g.:="" <span="">[SamplePage].\n\nThe wiki uses Markdown syntax.\n\n\</page>

Project Admins:

• Tim Van Steenburgh
• Guillermo Cruz
• Wayne Witzel III
• Kenton Taylor
• Cory Johns
• Pranav Sharma
• Dave Brondsema
• Alexander Luberg
• Deshani
• Heith Seewald
• Shalitha Suranga
• Dillon Walls
• Igor Bondarenko
• Rohan Verma

and while registering a new user in the forge the tg.log is also showing the same error
Please help me in fixing this