Defect #13487

Honor committer => user mapping in repository statistics

Added by Anonymous over 5 years ago. Updated about 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Baptiste Barth% Done:

0%

Category:SCM
Target version:2.6.0
Resolution:Fixed Affected version:2.5.2

Description

This patch changes the repository statistics view to honor the "commiter => user" mapping. Without it, the statistics page is pretty much useless for it.

After creating this patch, i discovered issue #2624 which is about the same issue, and also provides a patch; unfortunately it has been mostly ignored for 4 years.

I still decided to submit my own patch, because it has some advantages:
  • It properly deals with the case were a committer name has not been mapped to a user. The patch at issue #2624 seems to map such committers to the empty string.
  • It sorts case insensitive, which is quite useful for me.
  • It merges homonyms, as requested by #2065. Actually, this point could also be a drawback for some projects, namely if two distinct people sharing the same name work there. So perhaps this is not desired. I can change the patch to remove this, which will then only merge committers mapping to the same user.

repos-stats-map-users.patch Magnifier (2.56 KB) Anonymous, 2013-03-15 16:39

0001-Optimize-committers-users-map-retrieval-for-statisti.patch Magnifier (2.48 KB) Jean-Baptiste Barth, 2014-08-27 19:23


Related issues

Related to Redmine - Feature #2624: Repository statistics should honour user-mapping Closed 2009-01-30
Related to Redmine - Feature #2065: Commit graph should merge homonyms Closed
Related to Redmine - Feature #2761: Factorize user name in /repository/statistics Closed 2009-02-16
Related to Redmine - Defect #14338: svn: repository statistics issue when user names only dif... Closed
Duplicated by Redmine - Feature #7353: Map repository authors to Redmine users not used in stati... Closed 2011-01-17

Associated revisions

Revision 13351
Added by Jean-Baptiste Barth about 4 years ago

Move some RepositoriesController logic to Repository#stats_by_author (#13487).

Revision 13352
Added by Jean-Baptiste Barth about 4 years ago

Fix text failing on some rubies/sql drivers because of improper length in Change#action (#13487).

Revision 13353
Added by Jean-Baptiste Barth about 4 years ago

Honnor committers/users mapping in repository statistics (#13487).

Revision 13361
Added by Jean-Baptiste Barth about 4 years ago

Optimize committers/users map retrieval for statistic graphs (#13487).

Revision 13362
Added by Jean-Baptiste Barth about 4 years ago

Fix syntax for ruby 1.8.7 (#13487).

History

#1 Updated by Anonymous over 5 years ago

This patch also fixes issues #7353 and #2761 (which are duplicates of other issues, but nobody flagged them as such, and I don't seem to be allowed to flag issues as duplicates).

#2 Updated by Etienne Massip over 5 years ago

  • Category set to SCM

#3 Updated by Daniel Felix over 5 years ago

Well I would like to test this issue. But currently I'm at the hospital. Just in case of the description, this sounds to be worth to look at it. :-)

#4 Updated by Anonymous over 5 years ago

No worries. Get back to good health and return home from hospital, redmine is no nearly as important as that!

I just hope this patch won't share the fate of those others and still be here, unresolved (i.e. neither accepted nor rejected) in four years g.

#5 Updated by Daniel Felix over 5 years ago

Thanks!

Well I think this one will find a faster way. If there is the next minor release available this could be set to this release or at least the next major.

#6 Updated by Anonymous over 5 years ago

Sadly after 1 month, it seems nobody had a chance to look at this. Too bad :-(. It would be nice to get at least some feedback on these things... as it is, development of Redmine seems to happen mostly behind closed doors and is not very welcome to outside contributions :-(.

Perhaps somebody did look at this patch and found an issue with that -- but without any feedback, that's impossible to tell for me :-(.

#7 Updated by Toshi MARUYAMA over 5 years ago

Because this patch does not have tests, I don't know it is correct or not.
I think it is better that Graph logic is moved from controller to helper in order to test.
See: source:tags/2.3.0/test/unit/lib/redmine/helpers/gantt_test.rb

#8 Updated by Anonymous over 5 years ago

OK, that makes sense. I am not yet very familiar with the testing system (of Redmine, or any other Ruby / RoR app in general -- I am fairly new to Ruby). I will investigate the gantt_test you referred me to, thanks for the point. I will try and see if I can do something. Likewise, moving the logic from controller to helper is something I can do, if you prefer that.

However, currently I am swamped with other work, so it's easily possible that another month or two will pass before I get to address either point. If somebody else beats me to that with a better patch, I won't complain ;-) but in any case, the ball is now in my court, as I have some clear feedback. Thank you very much for that, and for your work on Redmine in general.

#9 Updated by Keats . over 4 years ago

Hi,
it's working great.
Thanks

#10 Updated by txemi M over 4 years ago

I would love to see this patch in next version,
thanks.

#11 Updated by Jean-Baptiste Barth about 4 years ago

  • Related to Defect #14338: svn: repository statistics issue when user names only differ in case added

#12 Updated by Jean-Baptiste Barth about 4 years ago

  • Tracker changed from Patch to Defect
  • Assignee set to Jean-Baptiste Barth
  • Target version set to 2.6.0

I started looking at it and adding some tests, I'll commit it soon, hopefully for an integration in 2.6.0 (it's a fix
but it has been here for a very long time, so it can wait past 2.5.3).

@Max: thanks for the proposals but I started again from scratch because, as Toshi said, this code should'nt be here (some code belongs to the model imho, some other to helpers for SVG rendering). Moreover your patch has many effects in one and I'd like to keep things in separate commits so Toshi or Jean-Philippe can easily revert this if they judge it's not correct. Anyway it helped me see where the problem is so thanks for your contribution !

#13 Updated by Jean-Baptiste Barth about 4 years ago

  • Related to deleted (Feature #7353: Map repository authors to Redmine users not used in statistics)

#14 Updated by Jean-Baptiste Barth about 4 years ago

  • Duplicated by Feature #7353: Map repository authors to Redmine users not used in statistics added

#16 Updated by Jean-Baptiste Barth about 4 years ago

Toshi MARUYAMA wrote:

Ruby 1.8.7 PostgreSQL test fails.
https://travis-ci.org/marutosi/redmine-bb/jobs/33601888

Thanks! That should be fixed in r13352. There's no check on Change#action length as it's not user-controlled but it seems SQL drivers don't react the same way so we may be better adding some...

#17 Updated by Jean-Baptiste Barth about 4 years ago

I added committers/users mapping in r13353, but I did not add the other features discussed by Max in the original issue (sort committer names, merge homonyms). In case tests pass, and if JPL and Toshi are OK with that, this feature could be released with Redmine 2.6.0.

Note that this can have a bit of performance impact because with this implementation multiple queries are run to find the correct user (it relies on Repository#find_committer_user while it could use Changeset#user_id directly). I'll improve that in the next few days.

Also it made me realize there are missing tests for Repository#find_committer_user and I think some edge cases are not handled correctly. I'll dig into that later.

#18 Updated by Toshi MARUYAMA about 4 years ago

Toshi MARUYAMA wrote:

Ruby 1.8.7 PostgreSQL test fails.
https://travis-ci.org/marutosi/redmine-bb/jobs/33601888

Because I restarted, log disappeared.
Ruby 1.9.3 PostgreSQL test fails due to same reason.
https://travis-ci.org/marutosi/redmine-bb/jobs/33601897

  1) Error:
test_stats_by_author_reflect_changesets_and_changes(RepositoryTest):
ActiveRecord::StatementInvalid: PG::StringDataRightTruncation: ERROR:  value too long for type character varying(1)
: INSERT INTO "changes" ("action", "branch", "changeset_id", "from_path", "from_revision", "path", "revision") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id" 

#19 Updated by Jean-Baptiste Barth about 4 years ago

Fwiw here's the patch to cut down the number of SQL queries by relying directly on Changeset#user_id. On my test repo (10k changesets, 40k file changes, 90 distinct committers), it cuts the number of SQL queries from 216 to 9.

But I don't commit it now because I'd like to have the feedback of the CI system which is down until the end of the week. I don't know if Oracle and SQLServer will support the custom select forms and I didn't see "AS" elsewhere in the codebase, so I want to be able to revert it quickly if it breaks.

#20 Updated by Jean-Baptiste Barth about 4 years ago

  • Status changed from New to Closed
  • Resolution set to Fixed
  • Affected version set to 2.5.2

All tests passing with r13361/r13362, so I close this issue. Other features deserve their own issue (but as said elsewhere I doubt they're a good idea in the general case for now ; maybe someday when we have something prettier client-side).

Also available in: Atom PDF