meeting-room-booking-system / mrbs-code

MRBS application code
Other
127 stars 63 forks source link

How do I restrict user from booking more than one room #2385

Open jberanek opened 5 years ago

jberanek commented 5 years ago

Hello, I'd like to know how can I restrict users from booking more than one room at the same period?

It's ok to have room x from 8:00 to 9:00 and room y from 9:00 to 10:00 for user z, but not both rooms together at the same time.

Reported by: sistemasifsc

Original Ticket: mrbs/support-requests/1682

jberanek commented 5 years ago

You'll have to create your own policy by editing the function mrbsCheckPolicy() in mrbs_sql.inc.

Original comment by: campbell-m

jberanek commented 5 years ago

I wrote this inside 'Check creation policies'. I think this will do, what do you think? Am I missing something?

$booking_user = $booking['create_by'];

// Get all of this user's booking
$sql = "SELECT start_time, end_time FROM $tbl_entry WHERE create_by=$booking_user";
$res = sql_query($sql);

for ($i = 0; ($row = sql_row_keyed($res, $i)); $i++)
{
    // Check if booking conflicts with any other booked rooms
    if($booking['start_time'] < $row['end_time'] && $booking['end_time'] > $row['start_time'])
    {
        $errors[] = get_vocab('room_period_conflict');
        break;
    }
}

Original comment by: sistemasifsc

jberanek commented 5 years ago

Yes, probably, though you need to (a) escape $booking_user and (b) enclose it in quotes:

$sql = "SELECT start_time, end_time FROM $tbl_entry WHERE create_by='" . sql_escape($booking_user) . "'";

It looks like you are using an old version of MRBS. It would be better to update to the latest version before customising it.

Original comment by: campbell-m

jberanek commented 5 years ago

Thank you so much, Campbell, it worked!

I'll talk to my superior and see if the people from IT can upgrade the system. I'm just an intern haha

Final code for my version:

$booking_user = $booking['create_by'];

// Get all of user's booking
$sql = "SELECT start_time, end_time FROM mrbs_entry WHERE create_by='" . sql_escape($booking_user) . "'";
$res = sql_query($sql);

for ($i = 0; ($row = sql_row_keyed($res, $i)); $i++)
{
    // Check if booking conflicts with any other booked rooms
    if($booking['start_time'] < $row['end_time'] && $booking['end_time'] > $row['start_time'])
    {
        $errors[] = get_vocab('room_period_conflict');
        break;
    }
}

Original comment by: sistemasifsc

jberanek commented 5 years ago

Oh I forgot one thing: how do I disable the user from using Control-Click to select more than one room?

edit: Found it, I knew I've read it somewhere.

Original comment by: sistemasifsc

jberanek commented 5 years ago

Yes (for the sake of anyone else reading this later),

$auth['only_admin_can_select_multiroom'] = true;

Original comment by: campbell-m

jberanek commented 5 years ago

Hey Mr. Morrison!

I convinced my team that we needed to update to the latest version and I'm trying to change this implementation to fit the newest code but it seems I'm missing something because the policy is not working anymore.

This is what I changed:

// Check room's period conflicts
// Here we won't allow that an user books
// more than one room per periods in the same day

// Get the name of the user booking the room
$booking_user = $booking['create_by'];

// Get all of user's booking
$sql = "SELECT start_time, end_time, room_name FROM mrbs_entry, mrbs_room WHERE create_by = ? AND mrbs_room.id=mrbs_entry.room_id";

$res = db()->query($sql, $booking_user);

for ($i = 0; ($row = $res->row_keyed($i)); $i++)
{
    // Check if booking conflicts with any other booked rooms
    if($booking['id'] <> $row['id'] && $booking['start_time'] < $row['end_time'] && $booking['end_time'] > $row['start_time'])
    {            
        $violations[] = get_vocab('room_period_conflict', $row['room_name']);
        break;
    }
}

Thanks for your attention!

Original comment by: sistemasifsc

jberanek commented 5 years ago

The SQL params need to be an array:

$res = db()->query($sql, array($booking_user));

If you are having problems working out what's going wrong then have a look in your PHP error log to see what error messages are being produced.

Original comment by: campbell-m

jberanek commented 5 years ago

I did the change but it keeps booking anyway. Where can I check this log?

Original comment by: sistemasifsc

jberanek commented 5 years ago

It's quite often a file called error_log in your MRBS directory. However it depends on your PHP configuration. If you look at the output of phpinfo() the error_log setting will tell you the file location.

If you can't find it then you can get errors displayed in the browser by adding the following at the bottom of internalconfig.inc.php:

error_reporting(-1);
ini_set('display_errors', '1');

If there are no errors you'll need to add some debugging code to your code to work out what's going wrong.

Original comment by: campbell-m

jberanek commented 5 years ago

Oof, I was testing everything as admin. Of course it wasn't working, admins have all rights haha

Thanks for the advice!

Original comment by: sistemasifsc

jberanek commented 5 years ago

Got it to work on the new version, just one more thing that is happening is that when there's a violation for period conflict(my policy), it redirects to the errors page and shows a message that the user can't book, all good, but I also get this text on top of the page:

E_NOTICE in C:\xampp\htdocs\mrbs_sql.inc at line 366
Undefined index: id

That's the $booking['id'] variable inside the if statement. I've tried to do an isset on it but it ignores the rule.

Any idea to avoid this?

Original comment by: sistemasifsc

jberanek commented 5 years ago

I think it must be the $row['id'] variable and not $booking['id']. The problem is that you are not selecting the id column in your SQL. Try

$sql = "SELECT E.id, E.start_time, E.end_time, R.room_name FROM mrbs_entry E, mrbs_room R WHERE create_by = ? AND R.id=E.room_id";

Original comment by: campbell-m

jberanek commented 5 years ago

I've just edited the SQL above to correct an error (I had R.room_id originally instead of E.room_id)

Original comment by: campbell-m

jberanek commented 5 years ago

By the way, you should really use $tbl_entry and $tbl_room instead of mrbs_entry and mrbs_room, in case somebody changes the table prefix. So it should be

$sql = "SELECT E.id, E.start_time, E.end_time, R.room_name FROM $tbl_entry E, $tbl_room R WHERE create_by = ? AND R.id=E.room_id";

and you will need to make sure that $tbl_entry and $tbl_room are declared as globals within the function.

Original comment by: campbell-m

jberanek commented 5 years ago

Thanks for the advice. I changed it to use global variables and I'm selecting id now.

I did a few tests and when calling get_vocab('room_period_conflict', X), I replaced X with $booking['id'] and $row['id'].

$row['id'] returns the correct id of the already booked room in the shown message but $booking['id'] doesn't. I even get the error Undefined index: id twice, the line of the if statement and get_vocab().

Original comment by: sistemasifsc

jberanek commented 5 years ago

I suspect $booking doesn't have an 'id' key at that stage because the booking hasn't yet been made in the database: you are still checking whether the booking is allowed.

Do you need the condition ($booking['id'] <> $row['id'])? You didn't have it in your old code. If you don't need it, then you also don't need to select the id.

Original comment by: campbell-m

jberanek commented 5 years ago

That's my suspicious too, I couldn't work around this issue.

I put this condition because when I edit a booking and try to save it, the policy 'period conflict' is triggered cause the edited booking I'm saving is the old booking in the same period. And with the condition, I can overwrite/save the edit.

Original comment by: sistemasifsc

jberanek commented 5 years ago

Oh, OK. In that case I think you need

    // Check if booking conflicts with any other booked rooms
    if(($booking['start_time'] < $row['end_time']) && ($booking['end_time'] > $row['start_time']))
    {  
      if(!isset($booking['id']) || ($booking['id'] != $row['id']))
      {
        $violations[] = get_vocab('room_period_conflict', $row['room_name']);
        break;
      }
    }

Original comment by: campbell-m

jberanek commented 5 years ago

Only got to test it today, it worked!

Thank you so much for your help!

Original comment by: sistemasifsc

jberanek commented 5 years ago

Hello dear Campbell, I've downloaded the latest version of the software, made my changes to the policies(the one in this thread) and it seems that our booking conflict policy is triggering, again, when I'm trying to edit a book (the latest change we've made).

Any thoughts?

Original comment by: sistemasifsc

jberanek commented 5 years ago

No, sorry, I can't think why it would be doing that. You'll need to insert some debugging code to find out what's happening.

Original comment by: campbell-m

jberanek commented 5 years ago

Alright, I'll do some tests. It's completely ignoring this condition:

if(!isset($booking['id']) || ($booking['id'] != $row['id']))

I thought something changed in the latests pushes to the default branch.

Original comment by: sistemasifsc

jberanek commented 5 years ago

I did some testings and by manually setting $booking['id'] as the same as the entry I'm editing, hard code, the policy doesn't trigger. But when I try to get the id from the variable $booking itself, it doesn't work.

From dumping the variable $booking, I wasn't able to see the id too, so I tested showing $booking['name'] in the error message to see if the variable is actually used in that moment and it worked for the name.

Images for reference: https://imgur.com/a/VX2fFXz

edit: I don't know where is the bug for $booking['id']

Original comment by: sistemasifsc

jberanek commented 5 years ago

Which version of the MRBS code are you using?

Original comment by: campbell-m

jberanek commented 5 years ago

Latest from the default branch

Original comment by: sistemasifsc

jberanek commented 5 years ago

The booking id is actually held in a variable $ignore_id. So I think the code you want is

    // Check if booking conflicts with any other booked rooms
    if(($booking['start_time'] < $row['end_time']) && ($booking['end_time'] > $row['start_time']))
    {  
      if(empty($ignore_id) || ($ignore_id != $row['id']))
      {
        $violations[] = get_vocab('room_period_conflict', $row['room_name']);
        break;
      }
    }

Original comment by: campbell-m

jberanek commented 5 years ago

Thank you so much for your attention, it is working properly now. The only change I made was to change $ignore_id to $ignore since the variable in the parameters of the function is only named as $ignore.

Have a wonderful day!

Original comment by: sistemasifsc