[jifty-devel] Jifty-DBI: patch proposal for a delete method for the Collection class

[Frederic BLANC]

On Thu, Oct 19, 2006 at 02:53:43PM -0400, Jesse Vincent wrote:
> 
[...snip...]
> 
> What would you think about making the method name something a little
> more explicit? 
> 
> $collection->delete_all_records() ?
> 
> 		->delete_records()?

Yes, delete_records() is more accurate than just delete().


> For something adding new SQL (especially new SQL that alters the
> database), tests are essential.

 I am trying to write some tests, but never use the Test::More module,
so I have to learn how to use it before...


> This is also, I think, the first time, users can alter the database's
> content from the collection class.

 Everything has to start some day ;-)  Maybe the delete_records() name is
misnamed too.  Perhaps removed_records_from_db() would be a better name
for that method?


> Given the way we use JDBI in Jifty,
> this would have the side-effect of walking around our access control.

 I concede you that if this method is used as it in Jifty, the access
control would be bypassed, but not all applications need to check
access rights per record-contents basis, most of them (IMO) just need
to check if the user can delete at the table level.
 And I think that the access control may be reintroduced by limiting
the collection to the records the user has the right to delete in most
of the case where access control has to be done regarding the
record-contents.


> This also doesn't quite have the effect I'd expect. 
> 
> If I create a collection, check that it has the right things in it and
> then call delete:
> 	1) I'm not sure I get the right result with regard to paging
> 	2) If someone else has added a matching row in the meantime,
> 	   I'm going to be a VERY sad user ;)

 If you create a collection using various criteria to match the right
things, then do a while ($item = $collection->next) { $item->delete },
I see a 3rd case where someone will be unpleased too: let's say that a
first collection is set like this:
1> $uselessTeam = Society::PersonCollection->new();
1> $uselessTeam->limit( column => 'team', value => 'useless');
1> while($fired = $uselessTeam->next) {
but because it is quite a big team, the following collection was defined
like this:
2> $cheapTeam = Society::PersonCollection->new();
2> $cheapTeam->limit( column => 'cost', operator => '<', value => 1000);
and run:
2> while($item = $cheap->next) { $item->team('cheap') }
before the first call to delete happened:
1>    $fired->delete
1> }
Of course, most of the cheap people were suppose to come from the 'useless'
team; I let you guess who is not going to be happy in this story ;-)

 With my method[*], when you set up your collection and check it has the right
things in it by going trough a ->next() loop, the delete will only happen
on the collection's records that still match the collection's initial criteria.

 [*]: with the corrected patch in attachement. 

> Would you be happy with a solution that called the records' delete
> methods instead?  That has the added benefit of catching model-specific
> delete code too :)

 Unfortunately no :( because for the application I'm writting, I really need
to be able to delete groups of records in one delete.

 Best regards,
 -F.

PS: I did some change for that new patch version that I did not have time
    to test yet.