767023 - Change the AITC client to use hidden instead of deleted

Status: VERIFIED FIXED

(Web Apps Graveyard :: AppsInTheCloud, defect)

Description

[Jason Smith [:jsmith]]

Per the changes in bug 766325, we need to modify the client to use hidden instead of deleted.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Change field name, v1

Low hanging fruit. I think I found all the references.

 grep -r deleted services/aitc/
services/aitc/modules/storage.js:   *  3. Mark all local apps as "to be deleted".
services/aitc/modules/storage.js:   *        deleted" set.
services/aitc/modules/storage.js:   *  5. For each app either in the "to be installed" or "to be deleted" set,
services/aitc/modules/storage.js:    // marking apps as deleted anyway. In a subsequent version (when the

No references remain in services/common in AITC server code.

Comment 2

[Jason Smith [:jsmith]]

I take it with this field change that I'll need to test with brand new accounts, right? Since probably apps I have existing for BID accounts on stage likely will have problems, right?

Comment 3

[Jason Smith [:jsmith]]

k9o nom - required client change to the server change

Comment 4

[Anant Narayanan [:anant]]

Should we instead create a tracker bug for first release of AITC and nominate that for k9o instead of every individual AITC bug?

Comment 5

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #4)
> Should we instead create a tracker bug for first release of AITC and
> nominate that for k9o instead of every individual AITC bug?

Let's talk at some point about this in-person (along with a few other things talk about as well).

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 635538 [details] [diff] [review]
Change field name, v1

Review of attachment 635538 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but -- as Jason implies -- isn't this change going to cause existing deleted records to no longer be hidden/deleted when processed by a client?

If backward compatibility is a goal, you're going to need to rework this.

See also one nit.

::: services/aitc/modules/storage.js
@@ +261,1 @@
>     *        deleted" set.

Did you deliberately not change the description of the set? ("to be hidden"?)

Comment 7

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #6)
> This looks fine, but -- as Jason implies -- isn't this change going to cause
> existing deleted records to no longer be hidden/deleted when processed by a
> client?
> 
> If backward compatibility is a goal, you're going to need to rework this.

Since AITC isn't on production servers yet (the default pref points to a stage URL), I /think/ this means its OK to not worry about what data exists in the wild.

> See also one nit.
> 
> ::: services/aitc/modules/storage.js
> @@ +261,1 @@
> >     *        deleted" set.
> 
> Did you deliberately not change the description of the set? ("to be hidden"?)

Correct. Locally, the app is being deleted, not merely hidden. It just happens to set a "hidden" flag on the record when it is deleted. Anant: can you confirm this?

Comment 8

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #6)
> > This looks fine, but -- as Jason implies -- isn't this change going to cause
> > existing deleted records to no longer be hidden/deleted when processed by a
> > client?
> > 
> > If backward compatibility is a goal, you're going to need to rework this.
> 
> Since AITC isn't on production servers yet (the default pref points to a
> stage URL), I /think/ this means its OK to not worry about what data exists
> in the wild.

That's correct, and is the main reason I wanted to land this in the server before anything started hitting production.

The data on the staging servers may disappear at any time and should not be relied on.  If the existing data on stage breaks stuff with this patch applied, we'll just clear the database and start over.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Ryan Kelly [:rfkelly] from comment #8)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > (In reply to Richard Newman [:rnewman] from comment #6)
> > > This looks fine, but -- as Jason implies -- isn't this change going to cause
> > > existing deleted records to no longer be hidden/deleted when processed by a
> > > client?
> > > 
> > > If backward compatibility is a goal, you're going to need to rework this.
> > 
> > Since AITC isn't on production servers yet (the default pref points to a
> > stage URL), I /think/ this means its OK to not worry about what data exists
> > in the wild.
> 
> That's correct, and is the main reason I wanted to land this in the server
> before anything started hitting production.
> 
> The data on the staging servers may disappear at any time and should not be
> relied on.  If the existing data on stage breaks stuff with this patch
> applied, we'll just clear the database and start over.

Makes sense. I think in order to verify this then, we might want to do that then if the existing data for staging test accounts becomes no longer valid. Is there a bug I should file somewhere for this? Or just mentioning it is enough on this bug?

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/dd0d02c1ee7c

Builds with this may not work until the servers are updated. Testers should coordinate with server operators.

Comment 11

[Gregory Szorc [:gps]]

Backed out for xpcshell bustage.

https://hg.mozilla.org/services/services-central/rev/4af0c62e30a7

I swore I ran the tests before submitting for review. :/ I guess I should have ran them harder.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Change field name, v2

Looks like code inside DOMApplicationRegistry referenced *deleted* as well. The only change in this patch is to change that to hidden as well. I think I can get away with updating code in that tree. But, submitting for explicit review so someone can explicitly approve me touching dom/apps.

Comment 13

[James Bonacci [:jbonacci]]

The corresponding server work is done and is in Stage.
It is scheduled to go to Production tomorrow - 6/26/2012.
REF: Bug 766325 - AITC: change "deleted" flag to "hidden"
https://bugzilla.mozilla.org/show_bug.cgi?id=766325

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 636422 [details] [diff] [review]
Change field name, v2

Review of attachment 636422 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sane to me.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/7b97f42c3a59

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

gps - Can you please comment on why you renomed this bug for k9o?

Comment 17

[Gregory Szorc [:gps]]

(In reply to Lawrence Mandel [:lmandel] from comment #16)
> gps - Can you please comment on why you renomed this bug for k9o?

I have no clue how I changed that flag. It was Bugzilla's fault!

Comment 18

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/7b97f42c3a59

Comment 19

[Jason Smith [:jsmith]]

Verified on OS X 10.7.