Deploy full scale oauth functionality

[TotallyNotChase]

Goal of the pull request:

• Documentation
• Bugfix
• Feature (New!)
• Enhancement

Description

Added full oauth functionality to the API. Now supports both app access tokens and user access tokens for all kinds of API calls. Closes #93

P.S - Long changelog, I hope you have some free time to read through all this :)

Changelog

• Updated app version to 2.8.7

• Updated dependency versions

• Refine file structure of utils (previously common-utils). buildURL.js has also been moved inside.

  Here's a list of functions that the utils include, corresponding to their parent files-

  • general.js

    • base64Encode
    • getSiteId
    • upperCase
    • isString
    • isEmptyObj

  • buildURL.js

    • buildSearchURL
    • buildShoppingUrl

  • URLQuery.js

    • encodeURLQuery
    • urlParseObj
    • constructAdditionalParams

• Changed structure of the ebay object. Most of the fields that were previously stored under this.options are now stored under this., this is done because the functions that rely on options are now easier to work with and should now follow a single responsibility principle, as well has avoid some confusion, hopefully.

  Most of the function calls will now require only this as a parameter, unless that function explicitly needs custom configurations or options provided by the user, in that case the config (similar to the original option) should be used

  The properties held withing this are-

  • environment
  • baseUrl
  • baseSvcUrl
  • oauthEndpoint
  • credentials
    • clientID
    • clientSecret
  • headers
  • globalID
  • siteID

  To elaborate on the previous statement about single responsiblity principle. Most of the functions called by the user will now use their own options parameter as config. So instead of feeding in this.options in each and every request (even though this.options might have some unwanted params), we'll only have to feed in the options the user gave us.

  For this reason, options like limit are no longer being accepted in the ebay constructor, these should be supplied to their respective function calls. Headers should be global though and that feature is still left intact, headers are now stored in this.headers.

  As for any huge paradigm shifts, there shouldn't be any. As mentioned before, most of the function calls that we were passing this.options to were only expecting a few of the fields in it, fields that are now easily found in this.

• The ebay constructor no longer uses the body parameter. Those are bound to their respective token getter functions.

• Rename getItem to getItemById and make it return a promise just like all the other functions.

• All the function calls (at this stage) that use an access token will now use this.appAccessToken. We've yet to implement any functions that use userAccessToken though.

• All the variables in the library that have a name that ends with Id have been changed so that all of them end in ID (both capitalized). This is for consistency, but this comes with a set of problems.

  Ebay's own API is very inconsistent in this topic, some fields use ID, while others use Id, to counter this, the fields that use Id (specifically all the functions that use urlParseObj and constructAdditionalParams will be taken care of using a regex replace. See here for clarity.

• Make the error messages consistent and remove custom styling. I believe it'd be helpful to make our custom exception classes rather than printing the exception flags such as (INVALID_REQUEST_PARAMS), let me know how you'd want to tackle this.

• makeRequest now takes in an additional parameter - customOptions, this is one of the few functions that require not just this, but also a custom config as parameters. this will still be passed in as self, which should be less confusing that using this.options as self and also a bit more concise. The self param is currently only used to extract the headers (this.headers), wheras customOptions should hold the contentType and the data (in case of a post request).

• Added postRequest, this is used only for POST requests and currently only by the access token functions.

I suggest we get rid of makeRequest and only keep postRequest and getRequest, with proper tweaks ofcourse. I believe this to be more concise.

[TotallyNotChase]

There seems to be a bug I missed on getUserDetails, I'll get on it!

[TotallyNotChase]

Additional Changelog

• buildSearchUrl now takes 3 parameters - self (aka this), config (aka options), and operationName
• Deleted includeSelector from input.includeSelector after it is used in buildShoppingUrl. This is done to avoid duplication of the same param in the resulting URL. First one is added by buildShoppingUrl, then another is added by stringifyUrl.
• I believe the links in shopping.test.js were using the wrong parameter capitalization. Most of those params use ID instead of Id according to the api docs. Here, here, and here. They are fixed accordingly now

[TotallyNotChase]

I added a bunch more fixes and added tests for the oauth functionality. Everything should work now. Though during development I encountered a few bugs that were left in the original branch.

There seems to be a few synchronicity issues. Since the previous logic was using this.options for every single call (which is a global variable), a lot of fields in that object were lingering. So a field from a previous call ended up being used in the current call. A prime example of this is in the finding.js file, in this line.

this assigns param to this.options but never assigns the name, which is required in this line.

So you'd assume that this'd result in options.name being evaluated to undefined and it should....except if you called findItemsByCategory previously, this.options.name will still linger due to this line. This probably isn't the intended behaviour.

This one should be fixed now, but I think there might still be some logic errors here and there.

[pajaydev]

@TotallyNotChase I appreciate your efforts to bring these changes. I will review this PR as soon as possible. I can see that you have included oauth changes and clean up task in same PR, Shall we separate that out ?.

[pajaydev]

@TotallyNotChase I don't think you have to change variable names as part of this PR, since this library is out and many people are using it. It would be difficult until or unless we have mention these changes as breaking changes and release a major version.

[pajaydev]

Shall we keep name as getAccessToken since we are using this convention from the start ?.

[TotallyNotChase]

@pajaydev ahh that's a fair point. My bad, I didn't realize that.

Would you like me to revert all the clean up changes (migration from this.options to this and its resulting changes, changed variable names, and changed function names) or revert only the changes that affect the front end user (variable names, function names)?

I agree that it makes much more sense to release a big cleanup + enhancement in a major release. Sorry for not realizing that.

[pajaydev]

@TotallyNotChase Yes, I appreciate if you can separate the enhancement (keep only Oauth changes) and the cleanup in two separate PRs, kindly keep all the variable name, this changes and function name changes in cleanup PR so that I can verify those changes. My point is, it's better not to change variable names or function names affecting users, until or unless its much needed.

[TotallyNotChase]

@pajaydev alright! I'll separate them out and make another clean PR :)