Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow custom_parameters as an object #30

Closed
wants to merge 2 commits into from

Conversation

nojhamster
Copy link

Fixes #29

This is a small change that allow using an object instead of an array as custom parameters, making reserved words (typically name) usable in parameters.

@nojhamster
Copy link
Author

Added some tests to comfort coveralls.

@simov
Copy link
Owner

simov commented Aug 24, 2015

Hi, @nojhamster, thanks for the feedback and for your effort to fix the issue. Your code helped me a lot. 👍

However, the main place for configuration is the JSON data structure. I assume you are setting your custom parameters programmatically, which is fine, but the user should be able to configure them through the JSON data structure. So, changes to the lib/config.js file were needed.

If you are interested in how I implemented it, take a look at this commit 6509568

With this fix you can now have the following configuration:

"trello":{
  "name": "my app"
}

You can pull master and test it. Let me know what do you think. The new version will be out by the end of this week.

Closed via 6509568

@simov simov closed this Aug 24, 2015
@nojhamster
Copy link
Author

Hi @simov, nice reactivity :) Setting name straight in the configuration overrides the provider name, ie. /connect/trello is replaced by /connect/<AppName>. However, I had a look at your changes and the new reserved word custom_params did the trick :

var grant = new Grant({ trello: {} });
grant.config.trello.custom_params = { name: 'MyApp' };

That way, I can connect using /connect/trello and have name=MyApp in the query string :)

@simov
Copy link
Owner

simov commented Aug 25, 2015

Yep, that's a very good use case, I definitely missed the name authorization parameter when I was adding the configuration for Trello.

@simov
Copy link
Owner

simov commented Aug 25, 2015

@nojhamster I got your point and the fix for the configuration is here 4e3c7bc

Basically I'm going to introduce the custom_params option to the end user as a safer way to define custom parameters:

"trello": {
  "custom_params": {
    "name":"my app",
    "expiration": "never"
  }
}

Now, technically this still works:

"trello": {
  "expiration": "never"
}

But you can't specify the name property that way. I was able to fix that as well, but I think having the custom_params option is a better solution in the long run. I'm going to update the docs as well.

@nojhamster
Copy link
Author

@simov I tried that last commit and it worked like a charm, I'm also of your opinion concerning the use of custom_params. Thanks !

@simov
Copy link
Owner

simov commented Aug 30, 2015

Version 3.5.0 is published on NPM - changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants