whimmel wrote on Thursday, August 06, 2009:
Could this instead have something to do with the new "calendar" column in users? It defaults to unchecked. I saw that it was unchecked in the CVS demo as well as my fresh install.
bradymiller wrote on Thursday, August 06, 2009:
hey,
Not possible, since the new "calendar" column in users code has not been committed to the 3.1.0 branch (And the bug is showing up in both the 3.1.0 cvs branch and the development tip cvs branch) :
3.1.0 branch cvs demo:
http://opensourceemr.com:2093/openemr
development tip cvs demo:
http://opensourceemr.com:2089/openemr
(both demos use following login credentials user:admin pass:pass )
-brady
whimmel wrote on Thursday, August 06, 2009:
Two ways I guess this can be fixed. One is to just ignore or disable the cookie-related stuff when restrict_user_facility is false.
The other is to fix the behavior of the cookie the way I meant the facility code to work. I’m not sure if I want to fight this battle :) I can see how this would conflict with a brand new demo install. But in real life:
1. admin is not a provider
2. no user created after installation would be without a default facility
Like the new calendar checkbox in the tip, the default facility (and the “schedule facilities”, aka users_facility table) are used together to determine if a provider should be on that facility’s schedule. See patient.inc:getProviderInfo() for $param2. This is called in pnuserapi.php around line 300.
If the admin doesn’t have a default or schedule facility then the admin shouldn’t appear on a calendar.
The appearing/disappearing of the calendar here with regards to searching happens because the search screen defaults to the first facility. That sets the post[‘pc_facility’] to 3. Which creates the cookie. The admin doesn’t have a default facility at all, so the calendar disappears. Search again under All Facilities (or delete cookies) and the post is 0, which resets the cookie and the calendar reappears because now it matches the default facility of zero.
In this case, IMO the correct one, the fix would ensure the calendar would *never* appear because 0 is not a valid facility.
If the admin user should have a calendar, then the admin user should be created with default_facility == 3 during installation.
*shields up* 
bradymiller wrote on Thursday, August 06, 2009:
hey,
I see what your saying.
I like the idea of making the cookies (both setting and using) optional. I’d rec making a new globals, ie use_facility_cookie, since some users may want to use this cookie stuff while also setting restric_user_facility to false (just place the toggles together in globals and label the section multi-facility since I am guessing there will be more in the future). I’m all for creation of new variables in globals.php, since in the future this will someday be migrated to the database (admin->config page), and the more setting options the better.
If the above is done, wouldn’t it then be simple to also not use cookies (both setting and using) if the pc_facility is set to 0?
Then simpleton users who are testing/using openemr as admin user won’t be turned off from openemr if they try out the calendar first (before changing user settings) and experience calendar disappearance.
The definitions of pc_facility is the default facility, so don’t see why we should force this to be something. Although, agreed there is no reason to go crazy here to make your cookie work with this since it’s only applicable for the admin user in users whom have not edited the admin user settings (will also go away in versions after 3.1.0 since the calendar needs to be turned on in users to see it anyways, thus setting a default user). So, just seems simpler to piggy back on the use_facility_cookie mechanism to turn off the cookies if pc_facility is set to 0.
-brady
whimmel wrote on Thursday, August 06, 2009:
Ok, I agree the facility choice being sticky via the cookie should be an option like you propose. And pretty easy to implement. I’ll put the check right on the call to setcookie() and delete an existing cookie otherwise.
However, I would still say that users.facility_id is a foreign key for the facility table and should line up that way upon installation.
bradymiller wrote on Thursday, August 06, 2009:
hey,
Sounds good. That would kill the bug, since option would be set to false by default.
Rod,
What do you think about the possibility of making facility_id line up upon installation (making it 3). I’m a bit worried about making these type of changes right before a release. Do you see any repercussions anywhere; I don’t want to do anything foolish.
-brady
sunsetsystems wrote on Thursday, August 06, 2009:
Offhand it appears to me that a zero value in users.facility_id is indeed an error that should be fixed. However code should also behave in a reasonable way in spite of such errors. The general rule is that newly introduced code should work as expected on its own, without creating new standards of behavior for other parts of the system.
tmccormi wrote on Thursday, August 06, 2009:
A question, what is special about the facility id 3? and why would you hard code that? I don’t even have a facility ID #3 in any of my sites. IF you are trying to create some kind of Master record for facilities then it should be done with a unique key field for the facility, ie: master_facility = true, or similar.
–Tony
tmccormi wrote on Thursday, August 06, 2009:
By the way, I agree that the admin should not be treated like a provider. Provider’s that need admin can have that added, but admin should not *have* to have access to medical information.
–Tony
bradymiller wrote on Thursday, August 06, 2009:
Tony,
Not hard-coded in per se. It’s all an issue in the install:
1) database.sql script creates a faculty table entry with an id of 3
2) the setup.php script creates the ‘admin’ user with a facility_id of 0
So, they don’t match up after install (this was a masked bug until the facility cookie came to be). And when the admin user is edited in any way, it will be forced to place an existent facilty_id number per the “defauly facility” entry. Won’t change upgrade or the running source code in any way.
This would be a vary easy fix in setup.php (give facility_id of 3 to admin user). Unless anybody has objections, I plan to do this. Should be sufficient time in the current testing cycle to show any problems.
Also, Tony, regarding the admin not being provider. Can’t the admin decide this after installing openemr. Giving the current initial admin all-encompassing access is useful so OpenEMR works out of the box, which I think is an important element in increasing our user base (along with easy install, upgrades, support, internationalization, and CCHIT certification).
-brady
tmccormi wrote on Thursday, August 06, 2009:
I don’t think it’s a problem to default to ‘3’, just curious where it came from. I don’t use the default "Your clinic here’ record ever, so I never noticed that.
Conceptually the admin could be restricted ex-post-facto, which is fine. Not sure if there are any special username based back doors. CCHIT security project will find those I’m sure
–Tony
bradymiller wrote on Friday, August 07, 2009:
I’m also curious why 3. I wonder what would happen if I change it to 2 or 1; I’m guessing openemr would spontaneously combust.
-brady
tmccormi wrote on Friday, August 07, 2009:
I think we could list that as a feature …
–Tony
bradymiller wrote on Friday, August 07, 2009:
hey,
This thing is more complicated than I thought. Very bizarre, but the ‘Default Facility’ sets different in the ‘new user’ screen and the ‘edit user’ screen. Check out the html source of each below.
openemr/interface/usergroup/usergroup_admin.php:
<td><span class=“text”>Default Facility: </span></td><td><select name=facility>
<option value=“Your Clinic Name Here”>Your Clinic Name Here</option>
</select></td>
For processing this forms sets only ‘facility’
openemr/interface/usergroup/user_admin.php
<td><span class=text>Default Facility: </span></td><td><select name=facility_id>
<option value="3" >Your Clinic Name Here</option>
</select></td>
For processing, this sets facility_id and facilty
Hence, setting a new user will set the following in the ‘users’ table:
facility_id=0
facility=‘Your Clinic Name Here’
Then when you edit the user it will set in the ‘users’ table:
facility_id=3
facility=‘Your Clinic Name Here’
So, to complete fix this bug, will need above setup.php simple change. And will need to replace pertinent stuff in openemr/interface/usergroup/usergroup_admin.php (chnge the GET facility and the facilty select stuff to :
if ($_GET[“facility_id”]) {
$tqvar = formData(‘facility_id’,‘G’);
sqlStatement(“update users set facility_id = ‘$tqvar’ where id = {$_GET[“id”]}”);
//(CHEMED) Update facility name when changing the id
sqlStatement(“UPDATE users, facility SET users.facility = facility.name WHERE facility.id = ‘$tqvar’ AND users.id = {$_GET[“id”]}”);
//END (CHEMED)
}
(can’t simply copy/paste it since structures different, but need to keep the same algorithm)
<td><span class=text>Default Facility: </span></td><td><select name=facility_id>
<option value="3" >Your Clinic Name Here</option>
</select></td>
Sound good?
Also, Bill. Is there a way to help keep code clear in future to use the new planned cookie globals toggle in all of the cookie statements (only like 5 total lines in two files, right?), so if even is cookie isn’t unset/deleted it will be ignored. I’m not sure about others, but any open cookie statement makes me sweat; would be nice to be clear that’s it’s turned off or on without needing to search through the code.
-brady
sunsetsystems wrote on Friday, August 07, 2009:
If I remember correctly, there was a time when users.facility_id did not exist; there was only facility. Which was of course ridiculous because that would break all the associations if the facility name changed. So facility_id was added with the intention of deleting the facility column – which should be done in the near future.
tmccormi wrote on Friday, August 07, 2009:
From a design perspective it silly to have a different form for Add/Edit, should just be a selection on the user list screen that opens the same program for add/changes.
It has confused every office I’ve trained so far.
–Tony
bradymiller wrote on Friday, August 07, 2009:
hey,
Modified below files to ensure ‘facility_id’ and ‘facility’ are set appropriately (committed to 3.1.0 branch and development tip). Note the ‘facility’ is still not set during setup.php for admin; no point since doesn’t cause bugs and were moving away from this mysql column anyways:
openemr/setup.php
openemr/interface/usergroup/usergroup_admin.php
Bill,
Let me know when your cookie toggle stuff is committed, and I’ll migrate it to 3.1.0 and restart the demos. If it’s not done by tomorrow, I’ll just do it (really easy mod to do) since planning another translation table release with lots of calendar changes, and want to ensure it’s working for the translators.
-brady
bradymiller wrote on Saturday, August 08, 2009:
Bill,
I just committed all the fixes to this bug.
I did the following:
1) made a toggle in globals for the cookie (default is false)
2) set it so the cookie could not set to ‘0’. (despite cleaning up bugs above, the search sets it to 0 if search ‘all facilities’; hence simple way to clean up the bug is not too allow setting the cookie to ‘0’).
It seems to work well in default mode and in your user_facility mode. Definitely look through and test it though.
Here are the files I changed:
openemr/interface/globals.php
openemr/interface/main/calendar/add_edit_event.php
openemr/interface/main/calendar/index.php
-brady
whimmel wrote on Saturday, August 08, 2009:
Thanks Brady, your #2 is how I was going to attack it. I’m sorry I didn’t get to it as yesterday I had to take an extra trip across the state. I’ll be sure to test and test