Faiulre in client creation in tip

OpenEMR Development
1

ytiddo wrote on Wednesday, June 02, 2010:

when creating a new client, if the first field on the ‘demographics’ that IS flagged with a ‘d’ indicating “check this field for dupes”, and it is an optional field (as it is for externalid in our openemr page), and you do not fill it in with any content, the URL is misconstructed for going to the duplicate checker, and you cannot add the client.

someone mixed their counting ordinals.

I’ve reconfigured dev to show the problem (just try to add a user without specifying an externalid), and the patch i just threw on the tracker against tip fixes it.

thanks to steven, for helping me figure out why the demos weren’t showing the same behavior.

Justin Doiel

2

stephen-smith wrote on Tuesday, June 08, 2010:

Please cross-link your tracker items and forums so it’s easy to find them.  Tracker Item: https://sourceforge.net/tracker/index.php?func=detail&aid=3010681&group_id=60081&atid=1245239

Also, your description on the Code Review tracker is a bit underwhelming.  It would help if you had clarified it a bit, as is there’s not enough information there to reproduce the bug, and there’s no link between the tracker item and this forum thread besides the ones we provide.

Brady or Andy, could you please give this a quick once over and commit it to CVS?  The patch is rather small.  Despite what tracker the item was created in, this is a bug fix, so it may need to be back-ported to rel-320 as well.  My employer is tracking this on our internal ticketing system and it would be great to close that ticket.  For reasons I won’t discuss, I’m not allowed to review or commit this patch.

Nitpick: “counting ordinal” is wrong.  Cardinal numbers are using for counting, referring to a set’s cardinality.  Ordinal numbers are used for identifying position in a sequence, referring to a set’s ordinality.  Cardinality Ordinality (Order Type)

3

stephen-smith wrote on Tuesday, June 08, 2010:

Wow, that sucks.  I changed what tracker it was in and SourceForge changed the URL.  New tracker item url: https://sourceforge.net/tracker/index.php?func=detail&aid=3010681&group_id=60081&atid=493001

4

stephen-smith wrote on Tuesday, June 08, 2010:

Issue appears to be introduced in commit cb436… when Rod was refactoring the duplication checking.  I’m not saying we would have caught this, but it is evidence that Rod should probably go through the normal review process rather than committing code to the tree without notice (like he did last night).

This was after the rel-320 branch, so I don’t think this bugfix applies to 3.2.0.

5

sunsetsystems wrote on Tuesday, June 08, 2010:

Sorry, I’ll try to start some dialog in the future about big changes like that.  Thanks for the patch.

Yesterday’s commit was bug fixes.  I’m not going through a “review process” for all of those.

Rod
www.sunsetsystems.com

6

stephen-smith wrote on Wednesday, June 09, 2010:

In general, I think very few patches, including bug fixes, are so important they can’t wait a few days for a proper review.  The goal of a review process is to prevent new bugs from going into the tree, and bug “fixes” are not exempt from programmer error.

That said, I understand the desire to get bug fixes in the main tree ASAP; it would be nice if you at least dropped a note in the forum indicating what bug you had fixed.  Also, if anyone doesn’t get a review (or some other feedback) in a reasonable time frame, you should assume no one has a problem with your patch and commit it. (After testing, of course.)

7

sunsetsystems wrote on Wednesday, June 09, 2010:

It’s not a question of importance, rather efficient use of time.  I can’t charge my clients for chit-chatting in the forums.  Any active developers will notice that the change was made, and will see the commit comments.  If someone wants to look at and suggest (or make) a change to a bug fix, they are free to do so.

Now if I had a habit of checking in buggy bug fixes then it would be a whole different story.  But I don’t.

The really important rule is, don’t check in untested changes.

Rod
www.sunsetsystems.com

8

stephen-smith wrote on Wednesday, June 09, 2010:

Participating in the project’s established policies and procedures for commits is not “chit-chatting”, it is how changes get integrated into the main code base.  If your clients don’t want to pay for that, make sure they understand the additional costs associated with large divergence from main code base.  It’s not entirely clear that this particular policy applies to you and Brady, but I’m assuming it does.

Static testing (a.k.a code review) is an important part of that process, normally-even for bug fixes, since they aren’t exempt from the problems that affect other patches.

I agree that you don’t have a habit of checking in buggy anything.  That doesn’t mean it’s impossible for you to make an error in your patches.  Waiting for a code review doesn’t prevent you from providing changes to your clients, and it rarely takes more than a 48 hours before someone addresses posted patches.  If it takes too long, you can always simply assume no one has an issue with the patch.  That forewarning period before the change occurs is incredibly valuable, and doesn’t cost any individual developer much.

The really important rule is, don’t check in untested changes.

Amen.

9

sunsetsystems wrote on Wednesday, June 09, 2010:

I believe the patch submission process you link to is for external submissions, and is not intended to apply to those who already have commit access.  That’s one reason we have a policy for granting commit access.

I am not willing to put obstacles in the way of bug fixing by established competent developers, and I do not agree with your assessment of benefit vs. cost.  Of course this is subject to change… when/if there is an actual problem, let’s fix it then.  Worrying about these things in advance is pointless.

Rod
www.sunsetsystems.com

10

ytiddo wrote on Wednesday, June 09, 2010:

Rod,

Where is this process for getting a CVS commit bit documented?

As someone who does not have one, despite getting code reviewed and accepted, other code reviewed and rejected, and having patches in the tracker, I’m quite interested to see what our official policy and procedure is.

This “what i commit is just good enough, no review required” policy is partially why i’ve effectively moved away from dealing with this project.

Where is the sign that says “code must be this good to enter”? because without a sign, I find myself very comfortable submitting code to a project. i want my code reviewed, i don’t want to be allowed to willy-nilly commit stuff. i want others’ code reviewed too.

Steven just chided me correctly, for not getting involved. It seems every time i try and get involved, we come to a conflict.
I’m meerly looking for a clear API for this project’s management, that I can code to.

Justin Doiel

11

sunsetsystems wrote on Wednesday, June 09, 2010:

Justin, the policy is that you need a history of submitting consistently acceptable patches that are in line with the project’s design and direction.  This comes down to a judgement call that the administrators (currently Brady and I) must make.  I’ll get with Brady and review your case.  Thanks for sticking with us!

Rod
www.sunsetsystems.com

12

sunsetsystems wrote on Wednesday, June 09, 2010:

Actually Justin, re-reading your message I’m not sure what you are asking.  Are you asking for commit access, or are you saying that all commits, no matter how trivial or from whom, should be peer-reviewed in advance?

Rod
www.sunsetsystems.com

13

stephen-smith wrote on Wednesday, June 09, 2010:

printf 'Testing...'
for i in 1 2 3; do
  printf '%d...' $i;
done
printf '\n'
14

ytiddo wrote on Wednesday, June 09, 2010:

Rod,

Thanks for asking for clarification.

As far as a request, no, i’d prefer not to have CVS commit. I don’t trust myself, or people in general.

Which is why yes, I’d prefer to have a review process in place, and would be MUCH more interested in being  able to review the work of others and to have the ability to get my code reviewed.

I do understand, and am a major advocate of the “trunk must work” philosophy, but I’ve seen a review process work so well on the coreboot project, that even i’m leaning toward a ‘all code should be reviewed, and someone else should sign off on it’ position.

Being involved in coreboot and seeing a process work so well is such a high contrast to how things are working here, where we have yet to have any real agreed to policy other than “me and brady make judgement calls”.

if/when a review process is put in place, I will be very interested. especially if i can get it outsorted, so i can subscribe just to the review list, and drop these forums. The current non-review process has me currently not filing bugs (with patches), because its just not worth the fight. Creating a patch to fix my problem is 30 seconds. posting it to a list? another 30 seconds. Logging into sourceforge, posting to a tracker, posting to the forum, then bugging everyone about it later… This has frustrated me to the point of just not bothering.

I hope this has made my request and position(s) (and probably opinions that shouldn’t be aired) clear.

Justin Doiel

15

sunsetsystems wrote on Wednesday, June 09, 2010:

Code reviews are usually a pain for me, and I’m very open to adopting tools and ideas that will make it easier and faster for everyone concerned.  Why don’t you start a new thread that goes into some detail with your suggestions for that.  Thanks….

Rod
www.sunsetsystems.com

16

bradymiller wrote on Saturday, June 12, 2010:

hey,

My thoughts on the matter and what we have been doing. The tracker code submissions have been for developers whom do not have commit access or developers (with commit access) whom want their code reviewed (to ensure technically sounds or ensure reasonable to add to the codebase etc.). We have not been getting all code and bugs reviewed. This gives developers with commit access independence, so for quick bug fixes or things they feel comfortable commiting they can do, or if feel need a review, then review. If it’s big, then should at least create a forum entry to discuss the plan before coding anyways. This judgement requires common sense, and is one of the bigger requirements for commit access( ie. demonstrate good common sense and some technical proficiency). 48 hours is actually a really long time; enough time for a developer to move on to something else and never commit the code, which then becomes uncommitable due to merge issues after awhile. Plus it’s very easy to review new code coming through github, and we are always amenable to reversing code if need be.  The push for new features and code is all in the context of fulfilling the meaningful use and security requirements needed to keep this project feasible in the future.  Also, we also only expect code submissions to follow the current codebase guidelines, and do not expect developers to improve the current codebase. This expectation will only frustrate new developers as a simple project balloons into craziness (for a recent example of this, see below thread):
https://sourceforge.net/projects/openemr/forums/forum/202506/topic/3725023

-brady

17

stephen-smith wrote on Sunday, June 13, 2010:

Input sanitization is important;  each time we don’t do it, it’s another possible SQL injection.  I don’t think it is appropriate to let code that doesn’t do sufficient input sanitization into the tree.  If a line in your unified diff starts with “+” (so, new or changed), it should do input sanitization.

Output sanitization is important; each time we don’t do it, it’s another possible XSS attack.  I don’t think it is appropriate to let code that doesn’t do sufficient output sanitization into the tree.  If a line in your unified diff starts with “+” (so, new or changed), it should do output sanitization.

Handling magic_quotes_* and register_globals correctly are good to do, but if the code you are replacing doesn’t handle them, you aren’t breaking anything new.  So, I’m fine with that.  It’s possible for register_globals to cause a security issue, but it hasn’t caused any so far.  If we do proper input/output sanitization, magic_quotes_* is at most an annoyance.

Outputting correct HTML 4 / XHTML 1 is already part of policy.

48 hours is NOT very long.  In 48 hours, our HEAD usually moves one or two commits it is rare for patches to break one or two commits later.  On the Git mailing list it was often a week before a proposed patch would make it into the master branch, and that was only if it passed review.

All that said, I was just trying to give a complete review of the code.  If you or Andy feel the code is ready to be committed as is, feel free to exercise your commit access.  I’m not ready to commit the code, because I don’t want to knowingly commit code with issues.  I’ve already addressed some of the issues I had with the code in my Git tree on Gitorious.  “review-3010456” is the name of the branch, it should be mirrored to GitHub after some time.

Once I think the patch is ready, I’ll post it for review and gladly wait 48 hours.  I know we can’t catch everything, but the fewer issues get checked in the less time we spend on fixing issues and the more time we spend on new features.

I’ll admit that “the perfect shouldn’t be the enemy of the good”, and that incremental improvement is necessary.  I’m just not comfortable with my commits containing issues that I know about.

18

bradymiller wrote on Monday, June 14, 2010:

Stephen,

In theory, I agree with everything you write and value highly your contributions to the project. Concrete examples of what to do for output sanitization should be put in the developer policies section; then easy to enforce on the code reviews by simply referring to it (like as is done on the Input Collection section). I agree new code should follow these guidelines, however, we should not require fixing this stuff in all code that is “touched” by a code submission. It’s great for developers to fix this stuff, but it requires a more intimate knowledge of OpenEMR code and a much higher investment of time by the developer; this will only provide a barrier to code submissions that deals with previous OpenEMR code.

Plus, on the 48H thing, not everybody here is as self-disciplined as you are. I have seen code get lost in the past as developers lose interest, so I generally try to get it (tested code, of course) into the codebase as quickly as possible.  A lot of this is developer style dependent, and probably better to remain flexible on this issue and give the developer (especially, if they already have commit privileges) autonomy on this. If the core developers at some point feel a more regimented means of code review/submission is needed, then can deal with that at that time.

-brady

19

stephen-smith wrote on Monday, June 14, 2010:

Ignoring generating javascript:
Output sanitization is basically limited to htmlspecialchars() usage.  I don’t think this is too much to ask for all touched code.  What good is policy if submissions aren’t required to follow it?  Doing output sanitization rarely requires knowledge outside of what can be concisely put in the policy.  (e.g. “htmlspecialchars() for anything in HTML that isn’t markup; ENT_QUOTES for attribute values.”)

On generated javascript:
Output sanitization for that is much tougher and can’t be done in isolation, as it requires the script to have the correct first and last lines.  Until we have some solid guidelines and helper functions, I’m fine with this no getting fixed; it doesn’t seem to be a large source of the existing XSS attacks anyway.

On barriers to entry:
There’s a reason rides have “you must be this tall” to ride.  The restraints are designed for a certain body size and going outside a fixed range can cause injury to the person.  Not enforcing those barriers to entry is unsafe.  Similarly, having minimum requirements for all touched lines in the code is done for good reasons.  Arresting security issues like SQL Injection and XSS attacks protects our users.  Not enforcing those barriers to entry is unsafe.  It seems to me like the project *needs* higher barriers to entry to make sure the code improves, instead of having systemic errors.

If a developer has commit privileges, then policy and the review procedure are really just guidelines.  Following them is preferable, but I’m not going to call for code to be reverted just because it didn’t follow P&P.  I don’t think anyone else would either.  If/when a bug fix has to be written against such code, I will feel justified to letting the community know that following P&P might have saved us time.

20

stephen-smith wrote on Tuesday, June 15, 2010:

Brady or Rod, could you please look over and commit this patch?  ytiddo doesn’t want me to review or commit it.  I think it looks fine and we have been using it in production for over a week.