Project

General

Profile

Forums » Open discussion »

Texuna Technologies fork at GitHub (continuation of #1650's issue-notes)

Added by Mischa The Evil over 16 years ago

Hello all,

Description

After starting discussion about Patch #1650 using it's issue-notes, I'll try to continue (and extend) discussion in this forum-thread hoping that it will become better traceable in future...

After I started discussion on patch #1650 Artem decided to publish Texuna Technologies'fork of Redmine at GitHub. Looking at it's README.rdoc I can see that it contains the following patches:
  • #1650 Start/end time tracking for timelogging
  • #1671 Show a breakdown of estimated/spent/remaining time for a version
  • #1717 Show diff for issue description change
  • #1705 Use Rails timezones support
  • #1680 Make version description multiline and textilizable
  • #1676 Only show incomplete target versions
and the following not patched things:
  • massive Trac migration improvements
  • User’s cross-project time log accessible from account page
  • Don’t fill start date for new issue

I'll also use this thread to extend the discussion to changes made in the redmine-fork in general later.

Fork investigation

Finally, after quite busy period, I found some time to investigate the fork more closely. This took me a lot more time since I had some serious problems getting a clean diff against a export of Redmine core @ r1778. After I finally managed to get that clean diff I investigated all the changes closely.
I made quite detailed log of this process which shows what has changed in the files and why it changed.

I'll attach this log to this message for referencing the code changes. This comes in handy cause most of the changes are made in one single commit thus making it impossible to distinct changes between eachother.

See further "Other questions regarding changes in the fork"...

Context-menu action errors

Intro

Regarding my secondary discussion about patch #1650 which is about:

After a while of using my vendor-branch including this patch I found another issue regarding this patch...
When using context-menu (right-click menu) in the issuelist, every change (even a more simpler priority-change) initiated through an action selected from the context-menu, brings me to the edit view for the issue showing me two errors saying:
« Project » is invalid
« Date » can't be blank

In this view the requested changes are preselected. When I now click submit the change gets applied though without any problems.

It looks like, if using the context-menu, those two parameters aren't passed to the controller somehow...

Behaviour of this error in your fork

While testing this with your fork (rebased against r1778) this still happens only while using the issuelist context-menu to change the issue's status.

When the default edit method is used the described error starts to show up. This error is initially introduced after r1689 which adds timelog custom-fields.
It is that revision which adds a new line 163 to the edit-method in ../app/controllers/issues_controller.rb which looks like:

@time_entry = TimeEntry.new

Revision r1765 refactors the context-menu heavily. Before r1765 most actions uses the edit-method. After r1765 most actions (except issue-status change [reason isn't clear to me at the moment??]) are using the bulk_edit-method.

The bulk_edit-method doesn't contain the line which is added to the edit-method in r1689. This results in my conclusion that the bulk_edit-method should be used for the issue-status change action in the context-menu.

When my proposed workaround is applied the issue's status can be changed using the context-menu without
  • recieving the problems mentioned at first in the issue-notes of patch #1650
  • recieving 0.0 timelog entries

Proposed workaround/fix

I found a way to solve/workaround this exact problem by using the bulk_edit method (which is defined in source:trunk/app/controllers/issues_controller.rb#L218) instead of using the default (single issue edit) edit method (which is defined in source:trunk/app/controllers/issues_controller.rb#L158) for the context-menu issue-status change links.

This change can be applied without any (unwanted) side-effects by changing the following loop in ../app/views/issues/context_menu.rhtml on lines 8-11 from:

       <% @statuses.each do |s| -%>
            <li><%= context_menu_link s.name, {:controller => 'issues', :action => 'edit', :id => @issue, :issue => {:status_id => s}, :back_to => @back}, :method => :post,
                                      :selected => (s == @issue.status), :disabled => !(@can[:update] && @allowed_statuses.include?(s)) %></li>
        <% end -%>

to
       <% @statuses.each do |s| -%>
            <li><%= context_menu_link s.name, {:controller => 'issues', :action => 'bulk_edit', :ids => @issues.collect(&:id), 'status_id' => s, :back_to => @back}, :method => :post,
                                      :selected => (@issue && s == @issue.status), :disabled => !(@can[:update] && @allowed_statuses.include?(s)) %></li>
        <% end -%>

I'd like to ask you to test this behaviour (and the proposed workaround/fix) and report your findings back into this forum-thread...

Your recommendations on #1650's discussion

Artem Vasiliev wrote:

Also you'll be welcome to try our other changes to Redmine, like diffs for issue descriptions (#1717) and remaining time calculation for version (#1671)

While disussing #1650 I already had those patched integrated into my private vendor-branch of Redmine. Both work fine and without any errors or problems.
Thanks for sharing those great patches... :-)

Other questions regarding changes in the fork

After investigating the diff between clean r1778 and this fork (changes upto 09/08/08) there are some questions raising in my head. I find some changes which are not clear to me why these changes are applied. This is also shown in the document I have attached which lists all the changes in your redmine-fork.
I'll list all changes which are unclear to me here hoping that you can provide the initial reason for the changes:

Outro

Now, I'll finish up this post. I hope it brings some light in the darkness regarding your great fork and the associated problems.

Greetings,

Mischa The Evil.

tt-master_changes.txt (13.4 KB) tt-master_changes.txt Changes between tt_master-09/08/08 against clean trunk at r1778

Replies (6)

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Artem Vasiliev over 16 years ago

Hi Mischa!

Thanks for a lot of work, in particular dissecting changes list from one big commit. I wish we used Git from beginning, and made commits to it from the start. Instead we commited to our local SVN repository, and I just didn't find clear way to move its commits to Git (moreover we would need to edit some of them before pushing to public because they had sensitive data like email addresses).

While testing this with your fork (rebased against r1778) this still happens only while using the issuelist context-menu to change the issue's status.

Strange, I've just tried changing issue Status through context menu on our instance - works fine, both with issues with timelog, without it, and 'in progress'. Did you try it with clean r1786 (now http://github.com/artemv/redmine_tt/tree/master is in sync with it)? As far as I understand you get the same error with it.

Best regards,
Artem

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Mischa The Evil over 16 years ago

Hi there again,

Replies:

Artem Vasiliev wrote:

Thanks for a lot of work, in particular dissecting changes list from one big commit. I wish we used Git from beginning, and made commits to it from the start...

Your welcome :-) Would it maybe be possible for you to retrieve a svn log which shows which files were changed for what reason only? So without what's actually changed inside the files?
Such a log would really help understanding what's changed in that one big commit here in the GitHub-repo (thus automatically answering the questions I asked you here: http://www.redmine.org/boards/1/topics/show/2352#8)...

Artem Vasiliev wrote:

...need to edit some of them before pushing to public because they had sensitive data...

Good thing you think about that... Do a quick search on the net and you'll find lots of public-repos containing sensitive data (even root passes etc.) :evil:

Artem Vasiliev wrote:

Did you try it with clean r1786 (now http://github.com/artemv/redmine_tt/tree/master is in sync with it)? As far as I understand you get the same error with it.

After these suggestions you gave me I started another thorough "investigation" using several versions utilising a strict path which would show where the error gets triggered without suffering from environmental-differences. See my (again) extensive notes below...

Test:

Test Case

When using context-menu (right-click menu) in the issuelist, issue-status change initiated through an action selected from the context-menu, brings me to the edit view for the issue(s) showing me two errors saying:

« Project » is invalid
« Date » can't be blank

In this view the requested changes are preselected. When I now click submit the change gets applied though without any problems.

Test Subjects

- Texuna Technologies'fork of Redmine (latest head of github-repo [commit: 177c5ac76913ad7576978f7946556b98392e741d] which is based (updated) to r1786
- Clean export of Redmine Core r1866 (latest head of Core-trunk, unpatched)
- Export of my vendor-branch (vendor-branch based on upstream's Core-trunk r1778 without proposed fix applied [http://www.redmine.org/boards/1/topics/show/2352#6] )
- Export of my vendor-branch (vendor-branch based on upstream's Core-trunk r1778 with proposed fix applied (http://www.redmine.org/boards/1/topics/show/2352#6] )

Test Procedure

1.1. unzipped into redmine dir (with right permissions)
1.2. created db using create database redmine character set utf8;
1.3. edited db-settings according to 1.2.

1.3.1. fixed (platform-specific) release showstoppers for TT-fork ONLY

1.4. rake db:migrate RAILS_ENV="production"
1.5. rake db:migrate_plugins RAILS_ENV="production"
1.6. rake redmine:load_default_data RAILS_ENV="production"
1.7. service httpd restart

2.0. logged-in as default admin
2.1. created a public test-project named "great" (http://path.to.redmine/projects/add) using all defaults
2.2. added myself (admin) to a member of the project in the role of Manager
2.3. created a test-issue (http://path.to.redmine/projects/great/issues/new):
  • Tracker: Bug
  • Subject: TestIssue
  • Description: TestIssue
  • Status: New
  • Priority: Normal
  • Assigned to: - blank -
  • Category: - blank -
  • Start: - blank -
  • Due date: - blank -
  • Estimated time: - blank -
  • % Done: - blank -

2.4. went to the issues-list (http://path.to.redmine/projects/great/issues)
2.5. alt + left-clicked (I'm using Opera) the newly created issue (which has current status "New") to open the context-menu
2.6. Selected "Status -> Assigned" (this is permitted within the default workflow loaded in 1.6)

Test Platform

Basic Info:

OS: CentOS 4.4 [RHEL derivative] based Linux Distro, using back-ported kernel from CentOS 5
Database: MySQL 4.1.20 (distro-specific patched)
Webserver: Apache 2.0.52 (distro-specific patched)
Ruby-Engine: Ruby Enterprise Edition 1.8.6-20080709
Rails-Engine: Phusion Passenger 2.0.2 (Rails 2.1.0)

Output of ./script/about:

    About your application's environment
    Ruby version              1.8.6 (i686-linux)
    RubyGems version          1.2.0
    Rails version             2.1.0
    Active Record version     2.1.0
    Action Pack version       2.1.0
    Active Resource version   2.1.0
    Action Mailer version     2.1.0
    Active Support version    2.1.0
    Application root          /path/to/redmine
    Environment               development (in fact production, but falsely reported due to Phusion Passenger implementation)
    Database adapter          mysql
    Database schema version   20080914222030 (this differs between several tested subjects)

Test Results

-----------------------------------------------------
|                               | Error Triggered?  |
-----------------------------------------------------
| TT-fork of Redmine (1)        |        YES        |
| Clean export of Redmine r1866 |        NO         |
| My vendor-branch (2)          |        YES        |
| My vendor-branch (3)          |        NO         |
-----------------------------------------------------

(1) = latest head of github-repo (commit: "177c5ac76913ad7576978f7946556b98392e741d":http://github.com/artemv/redmine_tt/commit/177c5ac76913ad7576978f7946556b98392e741d) which is based (updated) to r1786
        (To check this look at upstream's r1795; it isn't merged into TT's fork)
(2) = vendor-branch based on upstream's r1778 without proposed fix applied (http://www.redmine.org/boards/1/topics/show/2352#6)
(3) = vendor-branch based on upstream's r1778 with proposed fix applied (http://www.redmine.org/boards/1/topics/show/2352#6)

(BTW: I hope the TOC will be able to handle this thread... :-)

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Artem Vasiliev over 16 years ago

Hi Mischa!

Wow. I'm going to try that procedure )

Database: MySQL 4.1.20 (distro-specific patched)

Hm hm, we use MySQL 5.0.22 and it's runs that migration fine. I wonder can this test case be caused by using MySQL 4.1..

Interesting, why r1795 exists and why it causes 500 error in your case.. Something specific to Passenger/sub-URI config?

Would it maybe be possible for you to retrieve a svn log which shows which files were changed for what reason only?

Yes, sure, gonna make it..

Best regards,
Artem

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Artem Vasiliev over 16 years ago

Tried the procedure with our fork, got 'Successful update.' as usual. Can you please try it:
  • with FF instead of Opera;
  • if the error persists, Mongrel (instead of Passenger). We use 1.1.5
  • if the error persists, MySQL 5.0.22 instead of 4.1.20

Of cause the fact that you get the error with TT fork and doesn't on plain r1866 isn't pleasant to us; but at least let's dissect the environment bit it's reproduced in. To me it looks that most likely it's Opera or Passenger.

Best regards,
Artem

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Artem Vasiliev over 16 years ago

Ok, here it is.. Though it's bit too big to be useful, especially without diffs )

tt-redmine-log.xml (398 KB) tt-redmine-log.xml

RE: Texuna Technologies fork at GitHub (continuation of #1650's issue-notes) - Added by Mischa The Evil over 16 years ago

Hello again,

After some testing i'm back again... :-)

1.

Downgraded the maximum-length of version-description to 255 (http://github.com/artemv/redmine_tt/tree/master/db/migrate/20080902122007_change_versions_description_limit.rb#L3) due to problems during migration (DB type-limit)

Database: MySQL 4.1.20 (distro-specific patched)

Hm hm, we use MySQL 5.0.22 and it's runs that migration fine. I wonder can this test case be caused by using MySQL 4.1..

This is certainly a thing off MySQL < 5.0.3.

Quote from MySQL-manual:

"Values in VARCHAR columns are variable-length strings. The length can be specified as a value from 0 to 255 before MySQL 5.0.3, and 0 to 65,535 in 5.0.3 and later versions."

2.

Back-ported upstream's r1795 into TT-fork to get it working without 500 internal error (http://github.com/artemv/redmine_tt/tree/master/app/views/layouts/base.rhtml#L9)

Interesting, why r1795 exists and why it causes 500 error in your case.. Something specific to Passenger/sub-URI config?

  • r1795 exists because of:
    1. Issue #1186 exists
    2. r1748 was a first attempt to solve the issue
    3. r1771 reverts the changes from r1748 after complaints from edge-users
    4. r1786 introduces the changes from r1748 again after merging the outdated @hooks@-branch
    5. r1795 re-reverts the changes from r1748 after re-introduction in r1786
  • why it causes 500 error when r1795 isn't re-applied can be read in the notes of issue #1186
  • why it can't be a specific problem of my sub-URI setup at first is because I have ditched that setup. I switched to a setup which holds one app per subdomain (which is fact is a seperate apache vhost).

3.

Tried the procedure with our fork, got 'Successful update.' as usual.

So there must be a diff at some place... Hmm...

4.

Can you please try it:
  • with FF instead of Opera;
  • if the error persists, Mongrel (instead of Passenger). We use 1.1.5
  • if the error persists, MySQL 5.0.22 instead of 4.1.20
Ok, I've done the first two. I've now tried all combinations (client & server) using:
  • Mozilla Firefox 2.0.0.16 / 3.0.1
  • Opera 9.52
  • Internet Explorer 7.0.5730.11
  • Mongrel Gem 1.1.5
  • Apache 2.0.52 with Phusion Passenger 2.0.2 (containing Rails 2.1.0)

but the error persists heavily in all combinations.

I can't test the third suggestion easily due to the fact that I'm bound to the latest (stable) version of MySQL provided by my distro-vendor (which btw is ClarkConnect 4.3). Besides that limitation I am not very certain that the cause of the issue is lying in the different MySQL-version. It seems a bit unlikely to me, though I won't deny it at this stage ;-)

5.

To me it looks that most likely it's Opera or Passenger.

Agreed! Though it doesn't seem to be the cause of my issue after some more testing with Mongrel and Firefox...

6.

Ok, here it is.. Though it's bit too big to be useful, especially without diffs

Thanks very much... Indeed: it's big. Though it'll certainly help me finding out (mainly using the "Search"-function of the editor) why some changes has been made (based on the commit-descriptions, so it actually all comes down to your clarity in the commit-description :-)

Round-up:

Of cause the fact that you get the error with TT fork and doesn't on plain r1866 isn't pleasant to us

Thanks for the "concern" :-) Though atm I've chosen to fix this issue using my proposed workaround (note: link is broken -> #6 is pointing to sixth item in first message of this thread).
The workaround seems to work fine for me without any side-effects.
This situation verses the amount of time I have (invested already) made me decide to stick to the workaround since it looks like I'm the only one who is/stays affected by this issue and the fact the the workaround is quite small and without side-effects.

If you still need some more info regarding the issue just let me know. I'm willing to test whatever when possible (in fact not-bounded due to distro-limitations).

Last thing:

Would you be so kind to take a(nother) quick look at these unclarities and then post a raw brain-dump (so without spending much time on it) showing your thoughts regarding them?
It would be really helpful to bind those unclarities to commits using the big log-file.... :-)

Thanks again for all help and support...

Greetings,

Mischa.

    (1-6/6)