Merge lp://qastaging/~alecu/unity-lens-music/ubuntuone-webservices into lp://qastaging/unity-lens-music
Proposed by
Alejandro J. Cura
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Michal Hruby | ||||
Approved revision: | 122 | ||||
Merged at revision: | 122 | ||||
Proposed branch: | lp://qastaging/~alecu/unity-lens-music/ubuntuone-webservices | ||||
Merge into: | lp://qastaging/unity-lens-music | ||||
Diff against target: |
1247 lines (+1152/-3) 9 files modified
configure.ac (+3/-1) debian/changelog (+7/-0) debian/control (+2/-0) po/POTFILES.skip (+2/-0) src/Makefile.am (+5/-1) src/oauth.vapi (+19/-0) src/ubuntuone-webservices.vala (+342/-0) tests/unit/Makefile.am (+11/-1) tests/unit/test-ubuntuone-purchases.vala (+761/-0) |
||||
To merge this branch: | bzr merge lp://qastaging/~alecu/unity-lens-music/ubuntuone-webservices | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michal Hruby (community) | Approve | ||
PS Jenkins bot | continuous-integration | Pending | |
Review via email:
|
Commit message
Call every webservice with libsoup, using OAuth when needed
Description of the change
This branch adds code to call the Ubuntu One webservices needed by dash payments, using libsoup and OAuth. It adds a few unit tests too.
To post a comment you must log in.
56 + public unowned string sign_url2(string url, string? postargs,
Looking at the docs, it says that you need to free the return, so s/unowned//, plus the postargs should be "out string postargs" as it's also a return (out params are nullable by default).
118 + internal Soup.SessionAsync http_session;
"internal" is a special keyword used when building libraries in vala (internal symbols are callable by other parts of the library, but not by the consumers), pls use private/public as appropriate.
177 + public string nickname {
178 + get { return _nickname; }
179 + }
+ other instances
No need for the extra private variable, use "public string x { get; private set; }"
243 + credentials_ management. find_credential s ();
244 + yield;
Wouldn't it be better if find_credentials() itself was async? Then you wouldn't need the signals and you could just return the credentials / throw an error in the async method.
275 + internal virtual async void fetch_account () throws PurchaseError
Nothing wrong here, nice async method ;)
285 + var result = (string) message. response_ body.data;
278 + queue_signed_ webservice_ call ("GET", ACCOUNT_URI, (session, message) => {
You're using this multiple times, perhaps add a helper async method that takes method + uri and returns flattened Soup.MessageBody?
.data could be null, no? Or does the flatten() cause possible null to become ""?
576 + return 0;
You should return result of Test.run ()
852 + MainLoop mainloop = new MainLoop (); service. refetch_ payment_ info.begin( (obj, res) => { service. refetch_ payment_ info.end (res);
853 + purchase_
854 + mainloop.quit ();
855 + try {
856 + purchase_
857 + } catch (PurchaseError e) {
858 + error ("Can't fetch payment info: %s", e.message);
859 + }
860 + });
861 + mainloop.run ();
I remember you mentioned on IRC that you weren't sure how to do async tests with vala, so yep, this is the way, usually we just have a wrapper for the mainloop.run() which also adds a timeout, so the test fails eventually if there's no response. Usually using "assert (run_with_timeout (mainloop));"