New bug in CVS version

bradymiller wrote on Tuesday, July 14, 2009:

hey,

Likely from Jason’s changes today but possibly Rod’s.
If I create a new patient I get this after click ‘Create New Patient’ button:
white screen with following error:
Internal error: Attempt to change patient data with pid = ‘’ when current pid is ‘1’ for id ‘’

Using Mandriva Developer appliance:
Mandriva 2008
MySQL 5.0.45
PHP 5.2.4
php.ini settings:
short_open_tag = On
max_execution_time = 60
max_input_time = 90
memory_limit = 128M
display_errors = Off
log_errors = On
register_globals = Off
post_max_size = 30M
magic_quotes_gpc = Off (set this to off in my developer environments only)
file_uploads = On
upload_max_filesize = 30M
error_reporting = E_ALL & ~E_NOTICE & ~E_STRICT

-brady

bradymiller wrote on Tuesday, July 14, 2009:

Jason,
You reversed a lots of changes with your recent commits, since the patches were based on 3.0.1. I’d rec reverting all your commits and ensuring the changes are consistent with current cvs before committing.
-brady

tmccormi wrote on Wednesday, July 15, 2009:

We really need a way to provide changes  and patches against production releases.   Right now it  takes more than double the effort to provide / share enhancements. Since you have to get it working for your customers release then get it working again against a moving target CVS.     I’m sure those where my patches, which all work fine in 3.0.1 and I did say that was all they had been tested against, sorry Jason.

–Tony

bradymiller wrote on Wednesday, July 15, 2009:

hey,

I ended up reverting the following files, and then manually integrated them into cvs:
openemr/interface/main/left_nav.php
openemr/interface/main/finder/patient_select.php
openemr/library/patient.inc

It all seems to work fine.

To be blunt, this took up lots of my time, but i do think in the end this search mechanism will be worth it.  Tony, i’m gonna re-open the tracker for this patch with some recs to finish it up, since it’s sort of incomplete.

I think I mistakenly assumed you didn’t want these patches committed to cvs and was directed to current 3.0.1 users. If a patch is posted in the tracker please ask us explicitly to have us commit it to cvs if that’s what you want.  Then we can comment on what needs to be done (ie ensure integrated with most recent cvs base). Our goal is to commit these patches (or provide feedback if decide not to) as quickly as possible.

thanks,
brady

tmccormi wrote on Thursday, July 16, 2009:

Brady,
  Thanks.  The Tracker entry was two fold, 1 to provide it as a 3.0.1 overlay. and to to get just the feed back you mention on what would be needed to commit it to the development branch.   I’ll be more explicit next time and we will try and spend a bit more time on testing against the development branch as well.  Thanks again to both Jason and you.

The sooner we get this down the sooner the better.  We sould like be able to help you guys review and commit code.

–Tony

bradymiller wrote on Thursday, July 16, 2009:

hey,
Agreed. Since the volume of the patches is likely to increase soon, perhaps streamlining the patches by doing the following would help (just thinking out loud for now):
—As you did, list objective of patch, the files affected and duplicate this message in the developer forums.
—Submit the file as a patch created via the ‘diff -u’ command
—Use most recent cvs codebase as possible

The nice thing about including the files affected is that it will let other developers to stay away from modifying these files until your patch is committed. In the bigger projects, may also be useful to post a "patch" in tracker and forum even before the patch is actually created to let other developers know to stay away from modifying those files (would be nice to give a predicted date of patch release in those situations). Using above, we should be able to quickly review, test, and commit the patch (or give reasons why patch not ready for cvs yet).  I agree the review is where all the newer developers to openemr are gonna learn the most about openemrs coding principles (ie. access controls, internationalization, lists, layouts, widgets, billing etc.)

Once we get a process formalized, then can post it on the wiki.

-brady

tmccormi wrote on Thursday, July 16, 2009:

Sounds good.   I would add that patches against the stable release need to be easy for end-users to apply, I’m not sure diff -u is such a beast.  I agree that it is the right tool for developers to use to patch the current CVS Tip.

To do that right we have to have a stable patch branch for stable releases that is carefully controlled.

cfapress wrote on Friday, July 17, 2009:

Truly this mess was my fault. Tony did say his work was with the v3.0.1 code base and not CVS. As I described in an email to Rod and Brady, my method for integrating the changes made by Tony didn’t work as intended.

Unfortunately I didn’t have the time to extensively review the code submitted by Tony. I noticed at least several hundred line changes identified using ‘diff’. I made, what I thought was, a calculated decision and created a patch file by comparing Tony’s code with the CVS code.

Bad idea.

Or, perhaps I didn’t use the tool properly.

In either case, many thanks go to Brady for getting this straightened out.

Jason