Closed Bug 767023 Opened 10 years ago Closed 10 years ago

Change the AITC client to use hidden instead of deleted

Categories

(Web Apps Graveyard :: AppsInTheCloud, defect)

defect
Not set
normal

Tracking

(blocking-kilimanjaro:+)

VERIFIED FIXED
blocking-kilimanjaro +

People

(Reporter: jsmith, Assigned: gps)

References

Details

(Whiteboard: [blocking-aitc+], [qa!])

Attachments

(1 file, 1 obsolete file)

Per the changes in bug 766325, we need to modify the client to use hidden instead of deleted.
Depends on: 766325
Whiteboard: [blocking-aitc+]
Attached patch Change field name, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #635538 - Flags: review?(rnewman)
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?
k9o nom - required client change to the server change
blocking-kilimanjaro: --- → ?
Should we instead create a tracker bug for first release of AITC and nominate that for k9o instead of every individual AITC bug?
(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 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"?)
Attachment #635538 - Flags: review?(rnewman) → review+
(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?
(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.
(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?
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.
Whiteboard: [blocking-aitc+] → [blocking-aitc+][fixed in services]
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.
Whiteboard: [blocking-aitc+][fixed in services] → [blocking-aitc+]
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.
Attachment #635538 - Attachment is obsolete: true
Attachment #636422 - Flags: review?(rnewman)
blocking-kilimanjaro: ? → +
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 on attachment 636422 [details] [diff] [review]
Change field name, v2

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

Looks sane to me.
Attachment #636422 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/7b97f42c3a59
blocking-kilimanjaro: + → ?
Whiteboard: [blocking-aitc+] → [blocking-aitc+][fixed in services]
gps - Can you please comment on why you renomed this bug for k9o?
(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!
https://hg.mozilla.org/mozilla-central/rev/7b97f42c3a59
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [blocking-aitc+][fixed in services] → [blocking-aitc+]
Whiteboard: [blocking-aitc+] → [blocking-aitc+], [qa+]
blocking-kilimanjaro: ? → +
Verified on OS X 10.7.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-aitc+], [qa+] → [blocking-aitc+], [qa!]
Product: Web Apps → Web Apps Graveyard
You need to log in before you can comment on or make changes to this bug.