Merge lp:~mardy/ubuntuone-credentials/default-token-name into lp:ubuntuone-credentials

[Alberto Mardegan]

If no token name is given, use a default one

If no token name is given, use a default one

If we don't do something like this, then we need to accept this:
https://code.launchpad.net/~mardy/unity-scope-click/signon-plugin/+merge/288758
(which would be my recommendation, actually).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:246
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/156/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/33/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/156/rebuild

review: Needs Fixing (continuous-integration)

[dobey (dobey)]

After looking at this and the plugin-part1 branch again, I think the better solution here would be to get rid of the ValidateInput method completely here, and we should call buildTokenName() in the createToken() method where we assemble the JSON to POST to the server, instead of using m_data.TokenName() there. This value should also be set on the token and stored, rather than setting the value on the Token object after it had been stored. Then we should just read the value from the stored Token when one already exists.

It seems to me that would simplify the code a bit, and be aligned with the current behavior, which this change (and the current plugin-part1 code) is not.

review: Needs Fixing

[Alberto Mardegan (mardy)]

> After looking at this and the plugin-part1 branch again, I think the better
> solution here would be to get rid of the ValidateInput method completely here,
> and we should call buildTokenName() in the createToken() method where we
> assemble the JSON to POST to the server, instead of using m_data.TokenName()
> there. This value should also be set on the token and stored, rather than
> setting the value on the Token object after it had been stored. Then we should
> just read the value from the stored Token when one already exists.

But then we lose the possibility for applications to specify their own token name: every app running on the device will be using the same token name. Given that the server allows for different apps to specify a token name, I think we should preserve this possibility.

> It seems to me that would simplify the code a bit, and be aligned with the
> current behavior, which this change (and the current plugin-part1 code) is
> not.

I think that this change maintains the behaviour which we currently have, and it allows for a third party app to use a different token name.

In case it's not clear, the code in part1 causes the plugin to store the tokens as a dictionary:

  token-name-1: { token data 1 },
  token-name-2: { token data 2 },
  ...

[lp:~mardy/ubuntuone-credentials/default-token-name]

241. By Launchpad Translations on behalf of ubuntuone-control-tower on 2016-05-18

Launchpad automatic translations update.

[dobey (dobey)]

On Fri, 2016-05-06 at 09:32 +0000, Alberto Mardegan wrote:
> But then we lose the possibility for applications to specify their
> own token name: every app running on the device will be using the
> same token name. Given that the server allows for different apps to
> specify a token name, I think we should preserve this possibility.

This is fine for now. The goal here is to maintain feature parity with
the current implementation, while moving to the new signon back-end;
not to add more features that can cause problems, on top of that.

> I think that this change maintains the behaviour which we currently
> have, and it allows for a third party app to use a different token
> name.

It doesn't maintain the behavior, but extends it to allow alternate
token names.

> In case it's not clear, the code in part1 causes the plugin to store
> the tokens as a dictionary:
>
>   token-name-1: { token data 1 },
>   token-name-2: { token data 2 },
>   ...

There is much that isn't clear about this, which is why I think if we
do want such a feature, we should put it off until later. For instance,
how does the user know what the differences are? How are these
differences exposed in the UI? How are these managed separately from
the main account? There are many questions that need to be answered.

For simplicity sake, I think it's better to not do this now, and to
always force the same token name, as we have been doing for the past
2.5 years already.

[lp:~mardy/ubuntuone-credentials/default-token-name]

242. By dobey on 2016-05-24

Remove the superfluous validateInput method.
Use the same taken name always.
Store the token name as part of the token data.

[dobey (dobey)]

I've also just proposed https://code.launchpad.net/~dobey/ubuntuone-credentials/default-token-name/+merge/295628 which implements the token name usage as I suggested. Please have a look at it.

[Alberto Mardegan (mardy)]

Indeed, let's go with that one then.

Unmerged revisions

243. By Alberto Mardegan on 2016-05-27

Fix stored token response

=== modified file 'signon-plugin/tests/test_plugin.cpp'
--- signon-plugin/tests/test_plugin.cpp	2016-04-28 10:10:42 +0000
+++ signon-plugin/tests/test_plugin.cpp	2016-04-28 10:10:42 +0000
@@ -232,8 +232,8 @@
 
     QTest::newRow("empty") <<
         sessionData.toMap() <<
-        int(Error::MissingData) <<
-        false << QVariantMap() << QVariantMap();
+        -1 <<
+        true << QVariantMap() << QVariantMap();
 
     sessionData.setTokenName("helloworld");
     sessionData.setSecret("consumer_key=aAa&consumer_secret=bBb&name=helloworld&token=cCc&token_secret=dDd");

=== modified file 'signon-plugin/ubuntuone-plugin.cpp'
--- signon-plugin/ubuntuone-plugin.cpp	2016-04-28 10:10:42 +0000
+++ signon-plugin/ubuntuone-plugin.cpp	2016-04-28 10:10:42 +0000
@@ -69,13 +69,13 @@
     {
     }
 
-    bool SignOnPlugin::validateInput(const PluginData &data,
+    bool SignOnPlugin::validateInput(PluginData &data,
                                      const QString &mechanism)
     {
         Q_UNUSED(mechanism);
 
         if (data.TokenName().isEmpty()) {
-            return false;
+            data.setTokenName(Token::buildTokenName());
         }
 
         return true;

=== modified file 'signon-plugin/ubuntuone-plugin.h'
--- signon-plugin/ubuntuone-plugin.h	2016-04-28 10:10:42 +0000
+++ signon-plugin/ubuntuone-plugin.h	2016-04-28 10:10:42 +0000
@@ -54,7 +54,7 @@
         void userActionFinished(const SignOn::UiSessionData &data) Q_DECL_OVERRIDE;
 
     private:
-        bool validateInput(const PluginData &data, const QString &mechanism);
+        bool validateInput(PluginData &data, const QString &mechanism);
         bool respondWithStoredData();
         void emitErrorFromReply(QNetworkReply *reply);
         void createNewToken();