Bug in grid's edit validation.

[João Matos]

Hello,

I found a bug in the grid's edit validation.

I follow this procedure:

 - Select a record to edit.

 - Change one of the fields that must be unique and change it to a value that already exists.

 - web2py shows an error message (which is correct and happens before going to onvalidation, which is also correct).

 - Change the value to something that doesn't already exist in the table.

 - Mark the record for deletion.

 - Submit the record.

The bug I found is that web2py was supposed to go to onvalidation before deleting the record and that doesn't happen.

Can someone confirm this behavior and that it is indeed a bug?

Thanks,

JM

My env:

Windows 7 Pro x64 w/SP1+all upd

Firefox 66.0.2 x64

web2py 2.18.5 (2.18.4 showed the same problem)

Python 3.7.1 x86

[Anthony]

I found a bug in the grid's edit validation.

I follow this procedure:

 - Select a record to edit.

 - Change one of the fields that must be unique and change it to a value that already exists.

 - web2py shows an error message (which is correct and happens before going to onvalidation, which is also correct).

 - Change the value to something that doesn't already exist in the table.

 - Mark the record for deletion.

 - Submit the record.

The bug I found is that web2py was supposed to go to onvalidation before deleting the record and that doesn't happen.

If the record is being deleted, why would you need to validate the submitted values (given that they will not be applied)?

Anthony

[João Matos]

Because in my case the record is not deleted simply deactivated.

But there are other scenarios:

 - Making additional checks before allowing the record deletion;

 - Asking for the password of a supervisor before allowing the record deletion;

 - Making some changes to other tables as a consequence of the record deletion;

 - Wanting to receive an email when someone deletes a record from a special table;

 - Doing some house keeping or some other task when someone deletes a record.

There are 5 options when entering the grid's edit:

1. The user checks the record and decides to delete it without making any changes, selects the delete checkbox and submits.

2. The user makes some changes that are valid and then decides to delete the record and submits both the valid changes (which don't get saved) and the delete checkbox selected.

3. The user makes some changes that are not valid (eg. changing a unique field to a value that already exists) and submits. He receives the validation error message, corrects the value to a valid one and then decides to delete the record and submits with the delete checkbox selected.

4. The user makes some changes that are not valid (eg. changing a unique field to a value that already exists) and submits. He receives the validation error message, doesn't correct the value and decides to delete the record and submits with the delete checkbox selected.

5. The user makes some changes that are not valid (eg. changing a unique field to a value that already exists) and then decides to delete the record and submits both the invalid changes and the delete checkbox selected.

In 1, 2 and 3 web2py calls onvalidation. But not on 4 and 5 (which submit the same information, an invalid value and the delete checkbox selected). That is the problem here.

[João Matos]

I wrote "Because in my case the record is not deleted simply deactivated." and I meant "Because in my case I want the record to be simply deactivated instead of deleted."

[Anthony]

On Wednesday, April 10, 2019 at 4:04:35 PM UTC-4, João Matos wrote:

Because in my case the record is not deleted simply deactivated.

Whether or not the record is being permanently deleted or simply deactivated, it would not generally make sense to validate submitted changes, as the expectation would be that the changes would not be persisted. If you have a specific use case for allowing simultaneous update and deactivation (i.e., "save these changes and then immediately deactivate the record"), then you'll have to code that logic yourself (though I would recommend making the user experience more clear in that case, as the current UI and workflow does not make it clear that updates would be persisted prior to deletion). I don't think it makes sense for that to be the default behavior.

 

But there are other scenarios:

 - Making additional checks before allowing the record deletion;

That is what the "deletable" argument is for. It can be a function that receives a Row object and determines if the record is allowed to be deleted. If not allowed, (a) no delete button will appear next to that record in the grid, and (b) the "delete record" checkbox will not appear on the edit form.

 

 - Asking for the password of a supervisor before allowing the record deletion;

This kind of functionality would go beyond the scope of "onvalidation" anyway, as you would have to redirect elsewhere and start a new workflow. You might as well code your own logic to handle this.

 

 - Making some changes to other tables as a consequence of the record deletion;

 - Wanting to receive an email when someone deletes a record from a special table;

 - Doing some house keeping or some other task when someone deletes a record.

Better to use the db.mytable._after_delete callback for these cases.

Anthony

[Anthony]

I should also point out, at some point, if your requirements deviate too far from the built-in functionality of the grid, it might be easier to not use the grid and simply code your own solution.

[João Matos]

Thanks for the explanations.

You don't understand. I don't care if the changes made at the same time the deleted checkbox is selected, are saved when submitting. That is not the problem.

It is a question of consistency.

Either web2py should call on_validation when the delete checkbox is selected or not. What happens now is that in some scenarios it is called but not in others.

My opinion is that it should always be called, but above all, it should be consistent.

[João Matos]

They don't. I have everything working (the redirection to request for supervisor password on record deletion in the grid, using a link), the checks for record deletion in the edit form also work, except for the cases I pointed out.

[Anthony]

The "onvalidation" function is supposed to be called "upon validation" (hence the name), meaning after a successful validation of the submitted field values. It is not intended to be run in order to make checks before or after deletion of a record. In other words, onvalidation runs when particular conditions are met, and it does so consistently. It just so happens that you want to run it under different conditions, which is not its intended purpose. Even if it did run when you want, you would still need a way to distinguish a regular update request from a delete request, and you can do that just as easily outside of the onvalidation function (see below).

I think there are three types of cases to handle regarding deletes:

First, you might want to simply allow/disallow a delete, and that case is handled by the "deletable" argument.

Second, you might want to run some code after a successful delete. The grid has an "ondelete" argument to handle that in case the "Delete" button is clicked, but nothing for the case when a delete happens via an edit form. However, this can be handled in all cases via the db.mytable._after_delete callback. In any case, the onvalidation function would not be the best place for this logic as it (a) runs before the delete happens, and (b) would still require some logic to determine that a delete was requested. It might be nice if SQLFORM had a separate "ondelete" argument of its own.

Finally, you might want to allow the user to request a delete and then kick off a separate workflow, such as requiring a special password. However, that is really a special case that is application specific, and it is not even well accommodated by the current UI (i.e., it would be a better user experience to request the special password up front rather than making it a two step process). If you need something like that, you should really be implementing custom logic. And again, the onvalidation function is not the best place for such logic, as it would still require detection of the delete request.

As an aside, note that the "onvalidation" argument can actually be a dictionary with separate "onsuccess", "onfailure", and "onchange" callbacks. The grid also has "onupdate" (run after successful update) and "onfailure" (run after failed update) arguments, as well as the "ondelete" argument. Some combination of these callbacks could be used to run some code whenever a delete is requested, though it is probably simpler to just do a check like the following before calling grid():

if SQLFORM.FIELDNAME_REQUEST_DELETE in request.post_vars or 'delete' in request.args:
    [code to handle delete workflow]

 

Anthony

[Anthony]

On Thursday, April 11, 2019 at 7:54:46 AM UTC-4, João Matos wrote:

They don't. I have everything working (the redirection to request for supervisor password on record deletion in the grid, using a link), the checks for record deletion in the edit form also work, except for the cases I pointed out.

Yes, but just because everything is working does not mean (a) that was the easiest way to achieve the end goal or (b) you haven't compromised user experience to stay within the bounds of the available built-in functionality. For example, if a special password is needed to delete a record, it would be simpler to just enter it up front rather than first submitting the delete request and then having to enter the password on a separate page. Not only is that an unnecessary extra step, but it may surprise the user if the UI makes no indication ahead of time that a special password is required.

Anthony

[João Matos]

I'm sorry, but I can't agree with you about the consistency.

I'm aware that the onvalidation is called after the validation.

The inconsistency is why is it sometimes called when the delete checkbox is selected and not in others?

If the assumption is that it has nothing to do with deletion, then it should never be called if the delete checkbox is selected. Which is not the case.

There is a way to distinguish an edit from a delete from within onvalidation. I use

if form.vars.delete_this_record:

and it works.

I agree with your 1st case.

On the 2nd case, I must correct you that ondelete is run before the delete happens. I use it to mark the record inactive and stop the deletion, when the user "deletes" the record from the grid itself.

There is case I mentioned and that you didn't consider which is before the delete, doing something that doesn't require a separate workflow, which is exactly my problem. I just want to mark the record inactive. From the 5 scenarios I explained, all of which originate from the grid's edit form, 3 go to onvalidation (and are deleting the record) and 2 don't. Again, this is an inconsistency.

Thanks for the explanation about the arguments.

I tried with

if SQLFORM.FIELDNAME_REQUEST_DELETE in request.post_vars:

and this catches the missing cases.




But I must say it is a ugly solution. My point being that all the validation and checking is usually done in the onvalidation, ondelete, etc. And this to work must be at the top of the action instead of in one of the on* functions.

It works, but IMO is not a pretty solution.


[João Matos]

a) I find it the easiest to have all the validation and checking to be done in onvalidation, ondelete, etc.. Unfortunately this doesn't happen in 2 situations out of 5 for deletion (again the question of the inconsistency).

b) The asking of the password only occurs when the user decides to delete the record. If he enters edit to simply edit, there would be no sense in asking for the password. There is no extra step here.

[Anthony]

On Thursday, April 11, 2019 at 11:33:16 AM UTC-4, João Matos wrote:

I'm sorry, but I can't agree with you about the consistency.

I'm aware that the onvalidation is called after the validation.

The inconsistency is why is it sometimes called when the delete checkbox is selected and not in others?

If the assumption is that it has nothing to do with deletion, then it should never be called if the delete checkbox is selected. Which is not the case.

I agree it should never be called on deletion, but if it worked that way, it would make no difference for your use case. (As an aside, the reason the validation process proceeds even on deletion is likely due to the fact that SQLFORM is implemented by extending FORM, so the usual FORM processing happens before SQLFORM deals with the delete.)

I suppose instead you would like onvalidaton to run in all cases (when delete is checked), but I don't think that is the best solution for your use cases, as it would still require custom internal logic to distinguish deletions (not to mention that it would break backward compatibility). Instead, if we were going to make changes to accommodate these use cases, a better approach would be a separate before delete hook, and possibly an after delete hook. But even without these additional hooks, it is fairly easy to accommodate these cases with the DAL callbacks or simply with some code that runs before or after the grid code.

 

There is a way to distinguish an edit from a delete from within onvalidation. I use 

if form.vars.delete_this_record:

and it works.

Yes. The point is the need for such custom logic makes onvalidation less suitable for what you want to do. A separate callback would be more useful. Otherwise, you might as well just put that logic outside of onvalidation.

 

On the 2nd case, I must correct you that ondelete is run before the delete happens. I use it to mark the record inactive and stop the deletion, when the user "deletes" the record from the grid itself.

Yes, good point. Actually, in your case, I would probably simply disable the "delete" checkbox in the edit form (via editargs=dict(deletable=False)), and only allow deletion via the grid buttons (which you can then intercept via the ondelete callback).

An even better approach would be to use the db.mytable._before_delete callback, which is exactly how the built-in record versioning works (it adds a _before_delete callback that intercepts all deletes and instead updates the is_active field to False). This would catch deletion via the grid buttons or edit forms.

 

There is case I mentioned and that you didn't consider which is before the delete, doing something that doesn't require a separate workflow, which is exactly my problem. I just want to mark the record inactive. From the 5 scenarios I explained, all of which originate from the grid's edit form, 3 go to onvalidation (and are deleting the record) and 2 don't. Again, this is an inconsistency.

See above -- onvalidation is not the place for that logic. Though if you must, you can simply put that logic in both the onupdate and onfailure callbacks of the grid, or specify onvalidation as a dictionary, with onsuccess and onfailure callbacks that both include that logic.

 

Thanks for the explanation about the arguments.

I tried with

if SQLFORM.FIELDNAME_REQUEST_DELETE in request.post_vars:

and this catches the missing cases.




But I must say it is a ugly solution. My point being that all the validation and checking is usually done in the onvalidation, ondelete, etc. And this to work must be at the top of the action instead of in one of the on* functions.

It works, but IMO is not a pretty solution.


Callbacks are necessary when you need to run some custom code in the middle of some other process, but less important when your custom logic can run entirely before or entirely after the process in question. In that regard, as noted above, before and after delete callbacks in SQLFORM might be nice, but certainly aren't a necessity (particularly given that in many, if not most, cases, the DAL callbacks would suffice).

Anthony

[João Matos]

'll check out the db.mytable._before_delete callback.

You said

" I would probably simply disable the "delete" checkbox in the edit form (via editargs=dict(deletable=False)

), and only allow deletion via the grid buttons (which you can then intercept via the ondelete callback)."

ondelete works but has a drawback (at least I couldn't figure out a solution). It works fine if what we have to do doesn't require a redirection to another page. If it does, it doesn't work.

[Anthony]

On Thursday, April 11, 2019 at 2:03:00 PM UTC-4, João Matos wrote:

'll check out the db.mytable._before_delete callback.

You said

" I would probably simply disable the "delete" checkbox in the edit form (via editargs=dict(deletable=False)

), and only allow deletion via the grid buttons (which you can then intercept via the ondelete callback)."

ondelete works but has a drawback (at least I couldn't figure out a solution). It works fine if what we have to do doesn't require a redirection to another page. If it does, it doesn't work.

Deletes via the grid buttons happen via Ajax, so you need a client-side redirect:

def ondelete(table, id):
    redirect(URL(...), client_side=True)

Anthony

[João Matos]

Everytime I exchange messages with you I learn something. Thanks.