528388 - Delete branch data source not working on stage

Status: RESOLVED FIXED

(Socorro :: General, task)

Description

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Cannot delete a branch data source on stage.  Click delete, screen says it is successful.  Not removed though.  Delete works fine in a dev environment.

Comment 1

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Patch 1 for 528388

Found and patched the issue with delete.  Refactored while I was in there, to ensure that adds and updates had checks in place.  (If product_version is available, then update; if not available then insert).  The delete bug caused issues with adding/updating, with duplicate keys added to the database, so the refactoring should fix that issue as well.

Comment 2

[Frank Griswold [:griswolf] [:fgriswold]]

Comment on attachment 413190 [details] [diff] [review]
Patch 1 for 528388

Quibble: Comments in four places say 'The this (release|end) date for this product...' 

Better: Lose 'this'

Thinking about: Probably better not to delete a row from product_visibility unless the matching productdims row is also deleted? Instead mark ignore column True. This leaves some footprints in case anyone wants to figure out what is going on, while having the same effect as outright delete.

Comment 3

[Austin King [:ozten]]

(In reply to comment #2)
This isn't a minor detail. We need to firm up the rules around these tables...

As the patch stands now it deletes all data from
product_visibility
productdims
and these deletes cascade out and delete all the data from
top_crashes_by_signature
top_crashes_by_url

This seems like the right thing to do... please confirm.

Do we need a "ignore" action next to the delete action?

Do we need better copy around explaining what ignore and delete do?

(In reply to comment #1)
Patch is looking good. I like the refactoring. None of the SQL has names 
(/* soc.web branch.deleteProductVisability */ or whatever)

Also do we need a progress bar or maybe disable the submit button. When you delete a product with a lot of data in trend reports, then the delete takes a while.

Feedback I should have given in 1.1 - eventually we should have the delete redirect you to the branches screen. Right now after success if you hit refresh then it resubmits. Not a biggie for this patch.

Holding off on r+. I think we should resolve griswolf's concerns, but otherwise the patch looks ready.

Comment 4

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Thanks Frank.  I've updated the documentation in the Branch Model in my sandbox.

The delete() method calls the deleteProductVisibility().  These methods combined do:
1. Delete the record from the product_visibility table.
2. Delete the record from the productdims table.

Austin, I've added the /* soc.web ... */ documentation to each of the db calls in the Branch Model in my sandbox.

I was wondering why some of the deletes took so long.  Since the deletes cascade out to top crashes that makes sense.  I'll need to make sure the submit buttons are placed with some sort of status indicator.  Thanks for pointing that out.

Comment 5

[Frank Griswold [:griswolf] [:fgriswold]]

I'm getting an uneasy feeling about delete from productdims because of altering the mat views tables via cascade. Maybe in practice that won't matter: We will probably not have many productdims entries that are both in the mat views tables and need deleting.

We may want a Windows-ish "are you sure you mean it" before we let people delete, though.

Comment 6

[Austin King [:ozten]]

Please see a new related Bug#529923

Comment 7

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Attachment: Patch 2 for 528388

* Updated method header documentation. 
* Updated MySQL documentation.
* Replacing submit buttons with progress icons upon submit.
* Updated delete message to say "Yes, I'm sure..."
* Updated controller to redirect user to branch data sources page in order to prevent duplicate submission upon refresh.

Comment 8

[Frank Griswold [:griswolf] [:fgriswold]]

Comment on attachment 414345 [details] [diff] [review]
Patch 2 for 528388

comment starting at line 80 seems to be a cutty-paste error

function getProductVisibility can return at most a single row. Do you really want the return type to be array?

The function update at line 292 updates both productdims and product_visibility, but the comments are out of date about visibility. The comments *might* be right about what is wanted: If you just want to fix the gecko branch, for instance, you don't need to muck with dates; and much more likely, if you want to update dates, you don't need to muck with product/version stuff (there is a function updateProductVisibility for that. Good. It is private. Why?)

These are all quibbles. It looks good enough as is. Fix the quibbles and make it even better...

Comment 9

[Austin King [:ozten]]

Comment on attachment 414345 [details] [diff] [review]
Patch 2 for 528388

Thanks for making those changes. Looks good. 

Be sure to add view code during the commit (I saw show show hide JS but didn't see where it was called from).

Comment 10

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

Thanks guys.  Committed revision r1508.