1400016 - delegate what's new page decision into blobs

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), enhancement, P1)

Description

[[e-mail] (:bhearsum)]

What's New Pages are becoming the new standard whenever we ship a major release. The current way of configuring them (duplicating and slightly modifying a Release, and setting up an extra Rule) is complicated, error prone, and confusing. Catlee and I were talking today, and he threw out the idea of delegating the decision about whether or not to show the WNP in the blob, rather than always showing it. This would be pretty similar to how we make decisions about whether or not to return a partial, or which fileUrls to use - so I don't think it breaks any design goals.

In the past, we've talked about "property bags" as a way to get rid of the release duplication for WNP (https://bugzilla.mozilla.org/show_bug.cgi?id=933152), but this may be a better path forward because it eliminates the need for an extra Rule, which simplifies things for Release Promotion.

We'll need to handle a few special cases:
* Different (or no) WNP depending on channel
* Different (or not) WNP depending on incoming version. We should be able to use simple version comparison here - we don't need to try to match the updateQuery to another release in the database like we do for partials.

My first thought is to do something similar to fileUrls, eg:
{
  "name": "Firefox-56.0.1-build1",
  "openURL": {
    "release*": {
      "<56.0": "https://www.mozilla.org/%LOCALE%/firefox/56.0.1/whatsnew/?oldversion=%OLDVERSION%"
    }
  }
}

Note the "release*" (to ensure test channels also get the WNP), and the usage of "<56.0" to only show it for major version upgrades.

There could be better ways to structure it - the above is just off the top of my head.

Nick, I'm particularly interested in any thoughts you may have on this - I know you've thought a lot about this in the past.

Comment 1

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Jeff, I want to double check with you that this is OK for 56 and 57 - We'll automate showing the WNP to users on any major version bump.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

I think this could be a nice simplification of the rules, so long as we acknowledge that looking at the rules is no longer enough to determine WNP behaviour. Since user education is generally not enough we could add some marker/indicator to the rules page, for rules mapping to releases defining a WNP, and that WNP matches to the current rule filter. More requests and js logic is the likely cost of this, but worth looking at.

One other thing - in bug 1395788 comment #23 there is a more complicated WNP scheme than previous, offering a 57 preview page if the user starts before 56, and then a once-off mobile promotion to the first 56.0.x after that. We don't actually know how the once-off part will be implemented, so likely to change. The actual URL doesn't change though and bedrock switches content based on the oldversion query string. In your example I think we'd just replace the "<56.0" with "*".

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #1)
> Jeff, I want to double check with you that this is OK for 56 and 57 - We'll
> automate showing the WNP to users on any major version bump.

Note that while this would become the default behaviour, we'd still be able to remove it (or change the verions it's active for) before publishing.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #2)
> I think this could be a nice simplification of the rules, so long as we
> acknowledge that looking at the rules is no longer enough to determine WNP
> behaviour. Since user education is generally not enough we could add some
> marker/indicator to the rules page, for rules mapping to releases defining a
> WNP, and that WNP matches to the current rule filter. More requests and js
> logic is the likely cost of this, but worth looking at.

I'm not necessarily opposed to this, but I wonder what makes this special vs. other things that impact the update.xml that are decided by the Blob already (eg: fileUrls and partials)? Is it the fact that it's new (and therefore not something in the heads of releaseduty already)? Or is it that What's New Pages are user visible? Something else?

> One other thing - in bug 1395788 comment #23 there is a more complicated WNP
> scheme than previous, offering a 57 preview page if the user starts before
> 56, and then a once-off mobile promotion to the first 56.0.x after that. We
> don't actually know how the once-off part will be implemented, so likely to
> change. The actual URL doesn't change though and bedrock switches content
> based on the oldversion query string. In your example I think we'd just
> replace the "<56.0" with "*".

If I'm reading this correctly, the intention is that we always return the same WNP URL regardless of "from" version. If so, "*" would work. Or maybe "<56.0.1", if we don't want to add support for "*".

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #1)
> Jeff, I want to double check with you that this is OK for 56 and 57 - We'll
> automate showing the WNP to users on any major version bump.

I think so - eg this does not change what we're going to do, it just changes the details of how we do it.

Comment 6

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Ben Hearsum (:bhearsum) from comment #4)
> I'm not necessarily opposed to this, but I wonder what makes this special
> vs. other things that impact the update.xml that are decided by the Blob
> already (eg: fileUrls and partials)? Is it the fact that it's new (and
> therefore not something in the heads of releaseduty already)? Or is it that
> What's New Pages are user visible? Something else?

A bit of both really. Definitely user facing in a way that urls per channel aren't. And we've trained ourselves that WNP are done with an extra rule, so if it becomes invisible on the rules page it'll be easy to get confused. Some way of marking up a rule provides an alternative visual clue. Are you thinking we might end up with several markers if more functionality moves into the blob ?

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #6)
> (In reply to Ben Hearsum (:bhearsum) from comment #4)
> > I'm not necessarily opposed to this, but I wonder what makes this special
> > vs. other things that impact the update.xml that are decided by the Blob
> > already (eg: fileUrls and partials)? Is it the fact that it's new (and
> > therefore not something in the heads of releaseduty already)? Or is it that
> > What's New Pages are user visible? Something else?
> 
> A bit of both really. Definitely user facing in a way that urls per channel
> aren't. And we've trained ourselves that WNP are done with an extra rule, so
> if it becomes invisible on the rules page it'll be easy to get confused.
> Some way of marking up a rule provides an alternative visual clue. Are you
> thinking we might end up with several markers if more functionality moves
> into the blob ?

Yeah, pretty much. That's probably a road we should cross when we get to it though (if we ever get there, even...). Let's do the best we can for understandability now.

Comment 8

[[e-mail] (:bhearsum)]

I intent to start looking at this.

(In reply to Ben Hearsum (:bhearsum) from comment #0)
> We'll need to handle a few special cases:
> * Different (or no) WNP depending on channel
> * Different (or not) WNP depending on incoming version. We should be able to
> use simple version comparison here - we don't need to try to match the
> updateQuery to another release in the database like we do for partials.

A new wrinkle here is that we also want per-locale controls, and we may want different combinations of these depending on the situation. This requirement makes my initial idea of a nested dictionary less attractive - I'm going to consider other ways of doing this.

Comment 9

[[e-mail] (:bhearsum)]

I sketched out an idea for this in https://github.com/mozilla/balrog/compare/master...mozbhearsum:wnp?expand=1

In it, I've added support for this data structure at the top of a release blob:
    "updateLine": [
        {
            "for": {},
            "fields": {
                "appVersion": "31.0.2",
                "displayVersion": "31.0.2",
                "detailsURL": "http://example.org/details/%LOCALE%",
                "type": "minor"
            }
        },
        {
            "for": {
                "locales": ["de", "en-US"],
                "channels": ["release*"],
                "versions": ["<31.0"]
            },
            "fields": {
                "actions": "showURL",
                "openURL": "http://example.org/url/%LOCALE%"
            }
        }
    ]

As shown, it allows all <update> properties to be defined in that block, and each block has its own set of conditions. I haven't added support for globbing or any special matching yet - that will probably require some refactoring of the database layer code that already does it.

I didn't mean to, but this ended up looking very, very similar to the update status test config files (eg: https://hg.mozilla.org/users/bhearsum_mozilla.com/tools/file/5fdc71b233c3/scripts/updates/update-status-57.0.4.yml). It makes me a bit wary of my own approach after realizing this - there may be a better way of doing this still that I'm blind to at the moment. Using such a similar format seems like it would take away some of the usefulness of the update status tests, too.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: move rule matching helpers out of database layer

Comment 11

[[e-mail] (:bhearsum)]

Attachment: proof of concept for WNP (and more) delegation

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8941594 [details] [review]
proof of concept for WNP (and more) delegation

Very close, just a few comments on Github.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 8941594 [details] [review]
proof of concept for WNP (and more) delegation

I think this is ready for real review now.

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/b0a9bd8a70b5a327b957cb53446fa367f3612136
bug 1400016: Move rule matching logic out of the database layer to make it re-usable elsewhere. (#456). r=nthomas

Comment 15

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/87dec526c65a3ebb2c0b6152f627ec66335570b8
bug 1400016: add support for delegating what's new page decision making into AppRelease blobs (#457). r=nthomas

Comment 16

[[e-mail] (:bhearsum)]

This is now in production.