Expose OAuth access token and introduce new MediaWithTotal

[Yosif-Smint]

Hey,

I noticed that the original 'GetMediaListAsync' is missing total count functionality as per the documentation when we make a rest request and we set total = 1, the have an exception, since the returned model is different.

Another observation was that 'ActiveOriginalFocusPoint' could return 'double' instead of 'int'

The last one is a feature request, where the integration that I am doing needs to set explicitly an access token to the OAuthService.

Please consider my changes and suggest a feedback. Thanks!

CC: @Arpit-Sharma-USC

[coveralls]

Coverage increased (+0.5%) to 52.263% when pulling 790de2f on smintio:master into bf704f9 on Bynder:master.

[Arpit-Sharma-USC]

Hey @Yosif-Smint thanks for your PR. We'll take a thorough look at it asap. Although, I quickly skimmed through it.

I do have 1 change that I would request from the get go. It is do with the ActiveOriginalFocusPoint
We are aware of the possibility to change it to double. Recently one of our Tier-1 client faced an issue when we had changed it to a double. Perhaps they do a bunch of processing around the fact that this field is an int, the change lead to a lot of breaking changes on their end. For these reasons we had to revert the field back to an int. Could you please change that back to an int in this PR too?

Thanks!

[rh78]

Hello @Arpit-Sharma-USC - thanks for your feedback. Given your suggestion, how do you suggest we solve our issue when querying media (actually this bug renders your SDK unusable):

Newtonsoft.Json.JsonReaderException: 'Input string '266.5' is not a valid integer. Path 'activeOriginalFocusPoint.y', line 1, position 4425.'

In this case, on your side, you'd have to change your output format to never be double.

[Reonekot]

Just to chime in here, as I create half these changes my self yesterday and went to do a PR on most of the same. (Adding the ability to get the Total count etc.) Sorry for the a bit long post...

Regarding the ActiveOriginalFocusPoint - as rh78 wrote, this renders the SDK completely broken for us. If it warrants a major bump or what ever I can't really judge, but until this is changed to double/float (my local version uses float, but I don't know the data format used in Bynder...) we can't switch back to the Nuget version of the SDK.

IMO I think the naming / discoverability of these new APIs should be considered - I'm unsure if it's best to have overloads of GetMediaListAsync() with different input models. It might be, but "MediaListQuery" doesn't really tell me why I want that compared to MediaQuery (again, IMHO). Also my next task was to add the "count=1" version - and I'm a bit unsure how "we" want to expand the SDK to this. (As the return models will be different, as well as you can't combine total=1 with count=1.)
What do you think? How would the "count=1" endpoint look like/be named?

A couple of comments to the PR:
https://github.com/Bynder/bynder-c-sharp-sdk/pull/74/files#diff-1e68c8cf572918bd0679a0a07ca6f9e3b5d2f512140104a031e773c0851d6b0bR10. IMO it's weird to use int here and maybe consider bool with custom serializer (as used by lists etc.).

For the orderBy, I added the following comment (I can post another PR based on this if you want, but for now, I'll just post it here...)

        /// <summary>
        /// <para>Desired order of returned assets.</para>
        /// <para>See <see cref="AssetOrderBy"/> for possible values.</para>
        /// </summary>

And created the AssetOrderBy, similar to CollectionOrderBy:

namespace Bynder.Sdk.Model
{
    public static class AssetOrderBy
    {
        public const string DateCreatedAscending = "dateCreated asc";
        public const string DateCreatedDescending = "dateCreated desc";
        public const string DateModifiedAscending = "dateModified asc";
        public const string DateModifiedDescending = "dateModified desc";
        public const string DatePublishedAscending = "datePublished asc";
        public const string DatePublishedDescending = "datePublished desc";
        public const string NameAscending = "name asc";
        public const string NameDescending = "name desc";
    }
}

[Yosif-Smint]

Hi @Reonekot

Naming things is always hard :)

I agree that we should make this consistent.
Based on what I am seeing from the existing code base, one way we could name them is like this.

Task<IList<Media>> GetMediaListAsync(MediaQuery query);
Task<MediaTotal> GetMediaTotalAsync(MediaTotalQuery query);
Task<MediaCount> GetMediaCountAsync(MediaCountQuery query);

This is if we want to follow/preserve the existing approach.
I looked at the java implementation for naming inspiration, unfortunately no luck there either.

Again, I am not fully happy with this naming schema, however based on the Apiary documentation, other folks will be able to orient themselves.

I am open for suggestions, so we can move this along.
Please advise.

By the way I introduced the other changes that you suggested.
My intention is to touch as less as possible, so I am taking into account what is already out there.

I would also suggest that we should discuss 'ActiveOriginalFocusPoint' in a separate ticket/pull request.

Cheers,
Yosif

CC: @rh78, @Arpit-Sharma-USC

[Reonekot]

@Yosif-Smint - yes, as always naming is hard. But I agree, I think it's good to be able to have similarity with the docs. Also having the different options in the functions makes it more discoverable IMO, (I personally call it GetMediaWithCount(..) though, but not sure that's better...) though looking at again today, I'm not sure if it would be enough/better with basically just having the overloads with the different input models? Anyway, I guess we need more opinions on that one :)

I would also suggest that we should discuss 'ActiveOriginalFocusPoint' in a separate ticket/pull request.
Sure, makes sense - will you create an issue for it? As said, until this is fixed, we can't move back to the SDK in any case...

[Yosif-Smint]

@Reonekot, for me the returned response is count or counts grouped by the asset generic data and custom meta properties,
Anyhow, no ubiquitous language.
'GetMediaWithCount' makes sense for me. I can rename the other method to be 'GetMediaWithTotalAsync' and the returned response 'MediaWithTotal'.

Besides the naming, do you have any other suggestions on the pull request?
It would be great if other people chime in as well.

I will create an issue regarding 'ActiveOriginalFocusPoint'.

Yosif

[Reonekot]

Besides renaming to GetMediaWithTotalAsync, to match a coming (hopefully) GetMediaWithCountAsync, I don't have anything - looks pretty much what I have locally for this. :)

Ed: And thanks for opening the other issue as well - will be watching that to see what Support will say to the matter.