[FFE] Add payment preview for music

Why wasn't this landed before feature freeze?

Scott, It wasn't quite ready prior to feature freeze.

Then I think it should wait. We moved feature freeze later with the view that we would be stricter about exceptions. What's the impact of waiting?

Scott, If it doesn't go in Raring, we wait 6 months to get people making purchases via the Dash and lose out on a chance to make serious improvements based on the feedback we get from real world use.

Against the risk of releasing with something that isn't ready and is very buggy and/or upsets our users.

If you land it early in the s-series, you'll get feedback from real world use, and a chance to get it ship-shape before S releases.

@stefanor: This can't hurt much more than the last cycle's shopping lens fiasco, does it?

It should be clear that this code was ready before FF. It wasn't landed due to some needed code cleanup that couldn't occur before due to personal matters.

However, the code is well tested and really can't cause any regression (since it adds something wasn't possible to do before).

Em 14-03-2013 18:42, Marco Trevisan (Treviño) escreveu:
> However, the code is well tested and really can't cause any regression
> (since it adds something wasn't possible to do before).

Of course it adds a new feature, something that wasn't possible to do
before (that's why it needs a FFE). Anyway, this doesn't mean this new
feature don't have bugs (or, as in bug #1053470, another unity FFE,
privacy concerns).

I don't understand which benefits this new feature brings that overcome
the risk of introducing new bugs, with this 2000+ lines patch, and
justify a FFE?

2013/3/14 Marco Biscaro <email address hidden>:
> Em 14-03-2013 18:42, Marco Trevisan (Treviño) escreveu:
>> However, the code is well tested and really can't cause any regression
>> (since it adds something wasn't possible to do before).
>
> Of course it adds a new feature, something that wasn't possible to do
> before (that's why it needs a FFE).

What I mean is that is not changing code paths that we're currently
using, thus the possibility of having regressions is small.

My understanding was that bug 1153731 ( [FFe] Ubuntu One in the installer step ) is desired together with this FFe.

This too should wait. Feature freeze is really supposed to be the time this stuff is already landed.

There are a few reasons why this FFE is important.

1) This code, as mentioned before, was ready before FF but it was put on hold because of an urgent one-off project. It is fairly well tested both by my team in online services and by the unity team.

2) Because this code implements the payment previews it has to be part of unity for any other deliverables (via software center, or PPA) that may need it.

3) This can't be placed in a PPA, because it means putting all of unity in one, and makes a horribly invasive PPA

4) This code is not exposed at all. If we merge only this, and deliver the scopes that support purchasing separately, then this code is simply not visible to the user unless he installs that extra scope, and is effectively disabled.

Delivering in this way, with a FFE for this specific change, gives us the flexibility moving forward, while not endangering the user experience in any way.

Please reconsider.

Before considering this for a +1, can it go into the experimental unity7
PPA please?

@sabdfl working on it immediately.

Gave instructions to Robert to have this merged in the ppa and so that we can revert easily if things go wrong.

The good news is that the nux and libunity part were already merged weeks ago, so only Unity (the source) is impacted.

fitoschido, please do not change the status on release team process bugs like this. I'm putting it back to Won't Fix until such a time as the decision is changed through proper process.

Em 19-03-2013 07:44, Roberto Alsina escreveu:
> 4) This code is not exposed at all. If we merge only this, and deliver
> the scopes that support purchasing separately, then this code is simply
> not visible to the user unless he installs that extra scope, and is
> effectively disabled.

Does this mean that, even if the FFE is approved, the code will not be
largely tested by users until extra scopes are installed? It looks like
this may cause unnoticed regressions or bugs that will only be noticed
after the final release.

FYI: there will be some unity-lens-music changes modified and both should be merged into this FFe so that we can see clearly what's the impact and so on.

I asked yesterday them to:
- give a clear status of how the code is ready
- what components are exactly impacted
- attach them to this bug report
- rebase some on the 100scopes branch (to be able to test in the ppa)
- change the description to tell exactly what behavior/UI is changing

Once this is done, we can merge at once all the necessary components into the ppa, so that if things go badly, we can revert them easily (just one commit).

@Marco Biscaro: there is a choice here between a non-invasive, limited impact change, and a more extensive and visible one.

Talking with Didier Roche yesterday, we decided we'll propose the branches that expose the new functionality via the music scope. It should be up in a couple of hours or so.

> FYI: there will be some unity-lens-music changes modified and both should be merged into this FFe so that we can see clearly
> what's the impact and so on.
>
> I asked yesterday them to:
> - give a clear status of how the code is ready

What status would you like, I don't think I fully understand the statement.

> - what components are exactly impacted

The components that are affected are:
 - unity -> Where a new preview is added to allow the scope work with payments.
 - unity-lens-music -> Where the code is update to use the new API found in libunity to perform payments.

> - attach them to this bug report

Already did.

> - rebase some on the 100scopes branch (to be able to test in the ppa)

At least the unity code has already done this, alecu is working on the scope code.

> - change the description to tell exactly what behavior/UI is changing
Already did.

> Once this is done, we can merge at once all the necessary components into the ppa, so that if things go badly, we can revert
> them easily (just one commit).

re status:
the question was: is the code ready to land for you on those 2 components? Now that both components are indeed proposed for merging against the 100 scopes branches, I'm assuming it's a yes and that you tested that combination. :)

re description:
I still find the description not clear enough, it's just a global "we add payment from the dash". Can you please provide screenshotd and a clear manual test plan about what people should see, what's the difference exactly we should see to ensure the feature is working.

Meanwhile you provide those infos, once we get a first version of 100scopes landing in the certified ppa (meaning tests are passing), I'm happy to merge your 2 branches after upstream reviewed them (poking them right now) and have a test run.

Example of why we need a clear story and status on it:
Just saw that lp:~mandel/unity/error-preview was added to this bug. So it means:
- I don't know if the code is ready or not and tested against the 100scopes branches.
- we know have 3 branches instead of the 1 we had in the beginning. 2 being on the same component.

-> Please tell us if:
1. all this was tested against the 100 scopes branches
2. if everything is ready and we won't have another additional branch appearing in the next couple of days
3. merge both unity branches to just make one so that the in dash payment will just be one branch for reviewing per component and will make easier to revert if we don't have the +1 from sabfdl.
(there is still the 4: update the description, see my previous comment)

Until 1, 2, 3 are cleared, I can't mix the 100scopes testing with this one, not knowing if the feature is ready and tested.

> Example of why we need a clear story and status on it:
> Just saw that lp:~mandel/unity/error-preview was added to this bug. So it means:
> - I don't know if the code is ready or not and tested against the 100scopes branches.
> - we know have 3 branches instead of the 1 we had in the beginning. 2 being on the same component.

The reason to have two different branches against unity is to improve the quality of the reviews. As you can see lp:~mandel/unity/generic-payment-preview is a dependency of lp:~mandel/unity/error-preview . The reason for this course of action is to provide smaller code diff so that reviewers can focus better. I understand that the optimum thing to do is just have a single merge so that can be easily revert. I recommend to do the following, review lp:~mandel/unity/generic-payment-preview approve it when done and DO NOT MERGE it, then review lp:~mandel/unity/error-preview and merge it witch will bring the changes of the previous branch with it, that way we have a win win situation, better reviews and a single merge (once the later branch is merged lp will state that the previous one was too).

> -> Please tell us if:
> 1. all this was tested against the 100 scopes branches

We have create a ppa for the u1 QA team to test all this code with the 100 scopes code, we expect to have everything tested by EOD (time of the comment).

> 2. if everything is ready and we won't have another additional branch appearing in the next couple of days

Yes, all code present in those branches contains all the required changes.

> 3. merge both unity branches to just make one so that the in dash payment will just be one branch for reviewing per
> component and will make easier to revert if we don't have the +1 from sabfdl. (there is still the 4: update the description,
> see my previous comment)

I have answered at the top of the comment.

> Until 1, 2, 3 are cleared, I can't mix the 100scopes testing with this one, not knowing if the feature is ready and tested.

Understandable, we are going as fast as we can regarding the QA.

sounds like a perfect plan and matches what we discussed on IRC. Thanks! Keep me posted when we can have the green light (this doesn't block the review though) :)

Fix committed into lp:~unity-team/unity/libunity-7.0-breakage at revision None, scheduled for release in unity, milestone Unknown

Fix committed into lp:~unity-team/unity/libunity-7.0-breakage at revision 3091, scheduled for release in unity, milestone Unknown

So, FYI, the feature is merged in one commit per project with the 100 scopes feature. It will be easy to revert if we don't get the sabdfl +1.
https://launchpad.net/~ubuntu-unity/+archive/experimental-prevalidation

I ran the integration tests before merging those and then just after. Comparing, it seems we didn't regress anything (but the test coverage for the preview is really limited). Online Services QA did some testing on their branch + 100 scopes to confirm the feature is working.

Running the ppa here didn't seem to trigger regression.

What remains so that this feature can land in raring:
- some explanations on what happens when there is a payment transaction error, like credit card not being valid, network issue…
- sabdfl +1

> - some explanations on what happens when there is a payment transaction error, like credit card not being valid, network issue…

There are diff ways in witch the error will be shown to the user:

1. A preview with the error that occur will be shown in the dash.
2. A notification will be shown.

The reason to have both is to ensure that even if the user closed the dash (something that he can do at any time) he will be know about the error via the notification. If he does not close the dash both are shown but we considered that is better to be redundant than not being clear.

Update: Manuel is currently taking the branches for in-dash purchases out of the Unity branch (for bug #1154229) and putting it in a PPA where we can do further testing. We've already received some early testing feedback and found some issues that need to be fixed, so Manuel is doing that as well. Once the new PPA is up, we'll update this bug description with the details. We have QA lined up to test the new PPA. It's only after the new PPA is up and tested, that we'll present this to sabdfl.

after reviewing this feature further and discussing with Mark, we have decided to not push to land it for 13.04 as we ran into an authentication issue and it couldn't be determined in time whether this is a client or server issue. As this feature is dealing with sensitive information (credit card) we are taking our responsibility to not putting users at risk.
The feature will be further QA'd and will land in S as soon as it is deemed ready, we are still discussing whether to provide it via a PPA. An update will be available soon.

Fix released in Unity 7.1.0