search

jbbarth / redmine_better_crossprojects

Temporary plugin to handle usability problems discussed on redmine.org #5920
http://www.redmine.org/issues/5920
12 stars 8 forks source link

Code review before possible merge #6

Closed nanego closed 10 years ago

nanego commented 10 years ago

Can you please review this code? In this version, there is absolutely no dependence on the redmine_organizations plugin. Thanks!

jbbarth commented 10 years ago

Finished reading it, I made a few comments about style. The only real "production" blocker for me is the loading of all users, the rest is mostly syntactic sugar or details (which should be fixed too but not blocking).

Second blocker: I'd like at least 3 or 4 functional or integration tests, even in "OK" cases, so that we can be sure the code below is triggered when running the tests for the plugin. If we don't have this the plugin may break silently in future releases. (I must admit I'm not sure there was a lot of tests of that kind before, but now the view becomes very complicated so we need a safety net).

Thanks for you PR anyway, can't wait to see the modified one ;)

nanego commented 10 years ago

Here is the updated version. I added some functional tests and improved the code as per your recommendations. The load_users_map method has been modified, but it's still pretty slow and we need to work on it again before merging.

nanego commented 10 years ago

I updated this file so we can get (and cache) all users member of at least one project (opened or archived).

jbbarth commented 10 years ago

I just merged everything even if it's not perfect so that next pull requests will be easier to review (open new pull requests please, or commit directly on the master branch here)