Defect #13487
closed
Honor committer => user mapping in repository statistics
Added by Anonymous about 11 years ago. Updated over 9 years ago.
0%
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.
Files
| repos-stats-map-users.patch (2.56 KB) repos-stats-map-users.patch | Anonymous, 2013-03-15 16:39 | ||
| 0001-Optimize-committers-users-map-retrieval-for-statisti.patch (2.48 KB) 0001-Optimize-committers-users-map-retrieval-for-statisti.patch | Jean-Baptiste Barth, 2014-08-27 19:23 |
Related issues
Updated by Anonymous about 11 years ago
Updated by
Updated by Daniel Felix about 11 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. :-)
Updated by Anonymous about 11 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.
Updated by Daniel Felix about 11 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.
Updated by Anonymous about 11 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 :-(.
Updated by Toshi MARUYAMA about 11 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
Updated by Anonymous about 11 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.
Updated by
Updated by txemi M almost 10 years ago
I would love to see this patch in next version,
thanks.
Updated by Jean-Baptiste Barth over 9 years ago
- Related to Defect #14338: svn: repository statistics issue when user names only differ in case added
Updated by Jean-Baptiste Barth over 9 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).
Maxin Samuel: 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 !
Updated by Jean-Baptiste Barth over 9 years ago
- Related to deleted (Feature #7353: Map repository authors to Redmine users not used in statistics)
Updated by Jean-Baptiste Barth over 9 years ago
- Has duplicate Feature #7353: Map repository authors to Redmine users not used in statistics added
Updated by Toshi MARUYAMA over 9 years ago
Ruby 1.8.7 PostgreSQL test fails.
https://travis-ci.org/marutosi/redmine-bb/jobs/33601888
Updated by Jean-Baptiste Barth over 9 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...
Updated by Jean-Baptiste Barth over 9 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.
Updated by Toshi MARUYAMA over 9 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"
Updated by Jean-Baptiste Barth over 9 years ago
- File 0001-Optimize-committers-users-map-retrieval-for-statisti.patch 0001-Optimize-committers-users-map-retrieval-for-statisti.patch added
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.
Updated by Jean-Baptiste Barth over 9 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).