Change the AITC client to use hidden instead of deleted

VERIFIED FIXED

Status

Web Apps
AppsInTheCloud
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jsmith, Assigned: gps)

Tracking

unspecified

Firefox Tracking Flags

(blocking-kilimanjaro:+)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Per the changes in bug 766325, we need to modify the client to use hidden instead of deleted.
(Reporter)

Updated

6 years ago
Depends on: 766325
Whiteboard: [blocking-aitc+]
(Assignee)

Comment 1

6 years ago
Created attachment 635538 [details] [diff] [review]
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #635538 - Flags: review?(rnewman)
(Reporter)

Comment 2

6 years ago
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?
(Reporter)

Comment 3

6 years ago
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?
(Reporter)

Comment 5

6 years ago
(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+
(Assignee)

Comment 7

6 years ago
(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.
(Reporter)

Comment 9

6 years ago
(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?
(Assignee)

Comment 10

6 years ago
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]
(Assignee)

Comment 11

6 years ago
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+]
(Assignee)

Comment 12

6 years ago
Created attachment 636422 [details] [diff] [review]
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.
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+
(Assignee)

Comment 15

6 years ago
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?
(Assignee)

Comment 17

6 years ago
(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!
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7b97f42c3a59
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [blocking-aitc+][fixed in services] → [blocking-aitc+]
(Reporter)

Updated

6 years ago
Whiteboard: [blocking-aitc+] → [blocking-aitc+], [qa+]

Updated

6 years ago
blocking-kilimanjaro: ? → +
(Reporter)

Comment 19

6 years ago
Verified on OS X 10.7.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-aitc+], [qa+] → [blocking-aitc+], [qa!]
You need to log in before you can comment on or make changes to this bug.