Issue 6599 - Hotfix to drop updated page everywhere

I think we have definitively a better solution here so there is no point in keeping this opened.
https://codereview.adblockplus.org/29760565/

[a.giammarchi, 2018-04-23 07:42:23]

Since we are unable to properly address, or reproduce, the root cause of the
update page showing up repeatedly, but since this is a very annoying situation
for our users, I would like to propose this hotfix to avoid showing the update
page at all costs.

This change should not impact the rest of the planned release and it should give
us more time to properly drop the whole page and its related logic for the next
release.

[kzar, 2018-04-23 09:21:36]

Please could you add this review to the issue, mark it as under review and
assign yourself?

[a.giammarchi, 2018-04-23 09:36:44]

On 2018/04/23 09:21:36, kzar wrote:
> Please could you add this review to the issue, mark it as under review and
> assign yourself?

not sure how to mark it as under review but I've put the link and assigned it to
myself.

I haven't checked the `ready` yet 'cause I want to be sure Thomas and others are
OK with this hotfix as temporary measure to prevent issues on Friday release.

I hope that's fine.

[kzar, 2018-04-23 10:04:58]

On 2018/04/23 09:36:44, a.giammarchi wrote:

> not sure how to mark it as under review

You click the "codereview - Next status will be 'reviewing'." radio button when
modifying a ticket.

> but I've put the link and assigned it to myself.

Great, thanks.

> I haven't checked the `ready` yet 'cause I want to be sure Thomas and others
are
> OK with this hotfix as temporary measure to prevent issues on Friday release.
> 
> I hope that's fine.

Yes, that's right not to triage the issue (set priority and mark ready) since
you're not a module owner or peer of the Platform module. It's not clear we want
to go with your proposed change yet, but it's good to have it clearly linked on
the issue.

[Sebastian Noack, 2018-04-23 19:17:07]

https://codereview.adblockplus.org/29759558/diff/29759559/lib/subscriptionIni...
File lib/subscriptionInit.js (left):

https://codereview.adblockplus.org/29759558/diff/29759559/lib/subscriptionIni...
lib/subscriptionInit.js:251: updatesVersion > Prefs.last_updates_page_displayed)
Nit: Please also remove the now unused import of updatesVersion from the top of
the file.

[Sebastian Noack, 2018-04-23 19:22:55]

And we might want to update metadata.chrome too, to not include the unused
update page (and related resources) into the release.

And maybe we should also remove last_updates_page_displayed from lib/prefs.js.

[Thomas Greiner, 2018-04-24 09:31:29]

My impression was that simply removing the else-condition would be sufficient
for the hotfix.

[a.giammarchi, 2018-04-24 09:36:49]

On 2018/04/24 09:31:29, Thomas Greiner wrote:
> My impression was that simply removing the else-condition would be sufficient
> for the hotfix.

that is basically what I've done, I've dropped the else condition, hence grouped
the double if for the firstRun.

However, if the firstRun also could happen multiple times, I have the feeling
the hotfix should drop both pages until we are 100% sure what is going on.

Issue trac 6612 makes me indeed wonder if we should get rid of any automatic tab
creation for the time being.

I also have given up on trying to reproduce the issue in real to virtual machine
and browser I could try with any version between 3.0.1 and 3.0.3(or 4).

[Thomas Greiner, 2018-04-24 09:48:49]

On 2018/04/24 09:36:49, a.giammarchi wrote:
> On 2018/04/24 09:31:29, Thomas Greiner wrote:
> > My impression was that simply removing the else-condition would be
sufficient
> > for the hotfix.
> 
> that is basically what I've done, I've dropped the else condition, hence
grouped
> the double if for the firstRun.

That's fine with me.

> However, if the firstRun also could happen multiple times, I have the feeling
> the hotfix should drop both pages until we are 100% sure what is going on.
> 
> Issue trac 6612 makes me indeed wonder if we should get rid of any automatic
tab
> creation for the time being.

The first-run page is essential for UX and other reasons so while I agree that
we should find the underlying issue, we could at least help people who encounter
the updates page over and over again since most of those who were expected to
see it have seen it by now anyway.

[Sebastian Noack, 2018-04-24 09:58:48]

On 2018/04/24 09:48:49, Thomas Greiner wrote:
> On 2018/04/24 09:36:49, a.giammarchi wrote:
> > On 2018/04/24 09:31:29, Thomas Greiner wrote:
> > > My impression was that simply removing the else-condition would be
> sufficient
> > > for the hotfix.
> > 
> > that is basically what I've done, I've dropped the else condition, hence
> grouped
> > the double if for the firstRun.
> 
> That's fine with me.

I'd rather not leave dead code around. We don't know when/if we will reintroduce
the update page. But when we do it's trivial to just revert the changes here.

> > However, if the firstRun also could happen multiple times, I have the
feeling
> > the hotfix should drop both pages until we are 100% sure what is going on.
> > 
> > Issue trac 6612 makes me indeed wonder if we should get rid of any automatic
> tab
> > creation for the time being.
> 
> The first-run page is essential for UX and other reasons so while I agree that
> we should find the underlying issue, we could at least help people who
encounter
> the updates page over and over again since most of those who were expected to
> see it have seen it by now anyway.

Yes, the first run page is more crucial, also in terms of transparency, since
this is where most users learn about Acceptable Ads. So we cannot remove it.
Also there seems to be even less evidence that many users keep seeing the first
run page repeatedly.

[a.giammarchi, 2018-04-24 10:12:47]

I've applied all requested changes.

[Thomas Greiner, 2018-04-24 10:20:15]

Based on https://issues.adblockplus.org/ticket/6599#comment:16 we may want to
pause this review.

[Sebastian Noack, 2018-04-24 10:24:22]

https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome
File metadata.chrome (left):

https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome#old...
metadata.chrome:137: skin/updates.css = adblockplusui/skin/updates.css
What is about the graphical assets in skin/updates/*?

[a.giammarchi, 2018-04-24 10:37:56]

good point, removed skin/updates files too

https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome
File metadata.chrome (left):

https://codereview.adblockplus.org/29759558/diff/29760557/metadata.chrome#old...
metadata.chrome:137: skin/updates.css = adblockplusui/skin/updates.css
On 2018/04/24 10:24:22, Sebastian Noack wrote:
> What is about the graphical assets in skin/updates/*?

Done.

[Sebastian Noack, 2018-04-24 10:49:09]

Looks good to me, if we decide to move forward with removing the update page.
But as Thomas pointed out, we might as well want to handle the data corruption
and show a targeted message instead, now where we have new information.