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

Refactor apiUmbrellaWeb usage #1491

Merged
merged 3 commits into from
Sep 6, 2016
Merged

Conversation

jykae
Copy link
Contributor

@jykae jykae commented Sep 6, 2016

Closes #1485

Proposed changes

  • refactor createApiUmbrellaWeb to return wrapper object for Umbrella Admin API
  • refactor apiUmbrellaSettingsValid function based on new field names in Proxies

Please review @apinf/developers

@jykae jykae added this to the Sprint 30 milestone Sep 6, 2016
@@ -23,34 +24,25 @@ Meteor.methods({
}
},
'createApiUmbrellaWeb': function () {
const settings = Settings.findOne();
// Assume one proxy only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a TODO here mentioning that we need to refactor this method for multi-proxy support. That way we will remember what files to edit.

@brylie brylie self-assigned this Sep 6, 2016
@brylie
Copy link
Contributor

brylie commented Sep 6, 2016

@jykae this looks good. Please add the TODO comment, and we can merge the PR.

@jykae
Copy link
Contributor Author

jykae commented Sep 6, 2016

@brylie TODO added

@brylie
Copy link
Contributor

brylie commented Sep 6, 2016

I added a commit to clarify the comment. I.e. a TODO should be a verb, something that we have to do.

@jykae
Copy link
Contributor Author

jykae commented Sep 6, 2016

@brylie Ok, thanks. Anything else or could we merge this so I could use it in apiKey PR ?

@jykae jykae merged commit 8b67235 into develop Sep 6, 2016
@jykae jykae deleted the feature/construct-apiumbrellaweb branch September 6, 2016 10:06
@jykae jykae removed the in progress label Sep 6, 2016
@brylie brylie mentioned this pull request Sep 7, 2016
@ilarimikkonen ilarimikkonen added this to In Review in apinf/platform Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants