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.

Sign up for GitHub

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

Jump to bottom

Updated method getApiPath to take options as param #9937

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Updated method getApiPath to take options as param #9937

merged 2 commits into from
Oct 4, 2018

Conversation

rishabhgrg
Copy link
Contributor

@rishabhgrg rishabhgrg commented Oct 2, 2018 •
edited
Loading

Refs #9936

  • Updated methods to take single options param with version and type instead of separate values
  • Updated urlFor method to use the updated syntax

@rishabhgrg rishabhgrg force-pushed the api-path-config-update branch 2 times, most recently from 556e2e9 to 94afca1 Compare October 2, 2018 10:53
@kirrg001
Copy link
Contributor

kirrg001 commented Oct 2, 2018

@rishabhgrg Can you pls rebase and update? Thanks :)

@rishabhgrg rishabhgrg force-pushed the api-path-config-update branch 2 times, most recently from 3c07703 to 28fdf6f Compare October 2, 2018 11:38
@rishabhgrg rishabhgrg force-pushed the api-path-config-update branch 3 times, most recently from 27dd433 to 0a3e7b3 Compare October 2, 2018 12:26
kirrg001
kirrg001 reviewed Oct 2, 2018
View reviewed changes
core/server/services/url/utils.js Outdated
@@ -324,7 +327,8 @@ function urlFor(context, data, absolute) {
}

if (data && data.version) {
apiPath = getApiPath(data.version, data.admin);
let versionType = data.admin ? 'admin' : 'content';

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kirrg001
kirrg001 reviewed Oct 3, 2018
View reviewed changes
core/server/api/v0.1/index.js Outdated
@@ -115,7 +115,7 @@ const cacheInvalidationHeader = (req, result) => {
* @return {String} Resolves to header string
*/
const locationHeader = (req, result) => {
const apiRoot = urlService.utils.urlFor('api', {version: 'deprecated'});
const apiRoot = urlService.utils.urlFor('api', {version: 'deprecated', versionType: 'content'});

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@rishabhgrg rishabhgrg force-pushed the api-path-config-update branch 2 times, most recently from 2531dc4 to 8cf6127 Compare October 3, 2018 14:50
rishabhgrg
rishabhgrg commented Oct 3, 2018
View reviewed changes
core/server/web/parent-app.js
parentApp.use(urlUtils.getApiPath('deprecated'), require('./api/v0.1/app')());
parentApp.use(urlUtils.getApiPath('active'), require('./api/v2/content/app')());
parentApp.use(urlUtils.getApiPath('active', true), require('./api/v2/admin/app')());
parentApp.use(urlUtils.getApiPath({version: 'deprecated'}), require('./api/v0.1/app')());

This comment was marked as abuse.

Sign in to view

kirrg001
kirrg001 reviewed Oct 3, 2018
View reviewed changes
core/server/index.js
@@ -44,7 +44,7 @@ function initialiseServices() {
scheduling.init({
schedulerUrl: config.get('scheduling').schedulerUrl,
active: config.get('scheduling').active,
apiUrl: urlService.utils.urlFor('api', {version: 'deprecated'}, true),
apiUrl: urlService.utils.urlFor('api', {version: 'deprecated', versionType: 'content'}, true),

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@rishabhgrg
Updated method getApiPath to take options as param
8d64d11 
Refs TryGhost#9936

- Updated method to take single options param with version and admin instead of separate values
- Updated urlFor method to use the updated syntax
- Updated parent-app to use updated syntax
@rishabhgrg
Refactored method urlFor to use versionType instead of admin: true
154d4c3 
no issue

- Updated urlFor to work with versionType instead of admin:true
- Updated tests to use the correct method call
kirrg001
kirrg001 approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

Feel free to self merge 👍

We will improve some of the external url generation places next week. It's not perfect yet and we know that, but we delay the optimisation to next week

@rishabhgrg rishabhgrg merged commit dd151f3 into TryGhost:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirrg001 kirrg001 kirrg001 approved these changes

Assignees

@kirrg001 kirrg001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rishabhgrg @kirrg001