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

Settings API: move to primary document format #2606

Closed
ErisDS opened this issue Apr 17, 2014 · 5 comments · Fixed by #2661
Closed

Settings API: move to primary document format #2606

ErisDS opened this issue Apr 17, 2014 · 5 comments · Fixed by #2661
Assignees
Labels
affects:api Affects the Ghost API
Milestone

Comments

@ErisDS
Copy link
Member

ErisDS commented Apr 17, 2014

This issue is a result of the API format discussion in #2362, and is part of a larger project to move our API towards the JSON-API format which is documented in the Epic: #2124. This is one step in the journey towards achieving the Settings JSON object format laid out in #2350.


The equivalent task for Posts (#2580) has been done with the PR #2596, however the task for settings is SIGNIFICANTLY different.

At present, the settings API responses are completely different to all of our other APIs. The response when requesting a single setting looks like:

{
    key: "title",
    value: "Ghost"
}

And when requesting a collection, you get a response like:

{
    title: "Ghost",
    description: "Just a blogging platform.",
    email: "john.blogs@myblog.com"
}

The aim of this task is to make these responses consistent with all of our other JSON-API-esque responses. That is, return the full object (with anything sensitive omitted), with a plural-form named key:

{
    settings: [{...}, {...}]
}

In terms of the API itself, this is going to involve ripping out a LOT of code, but the settings API is the most heavily used, and so will require a lot of work updating the various places where it is called.

The settings API is also the only are which has an in-memory cache, which should also be transformed to work with the new format.

The request format should also be changed, meaning the client side models and code for saving settings will need adjusting.

The weird hacks for adding availableThemes and availableApps can remain for the time being, until the work on the individual Apps and Themes APIs are completed.

Finally, all the tests should be updated, and coverage should be added where it is missing.

It would also be appreciated if you would add docs to this (#2125) as you go. Thanks 👍

@jgable
Copy link
Contributor

jgable commented Apr 24, 2014

Question from over at #2061

  • Currently the browse implementation returns by type, should it be changed to match the rest of the API where it will return all?
  • If the browse() request is not internal, should this throw an error since it will return core settings?

@ErisDS ErisDS assigned jgable and unassigned ErisDS Apr 24, 2014
@ErisDS
Copy link
Member Author

ErisDS commented Apr 24, 2014

Sorry for the confusion here, but I think these questions are very specifically about #2061. This issue should change the format, not what the API does.

@ErisDS
Copy link
Member Author

ErisDS commented Apr 24, 2014

One thing worth noting - I guess if the request is for a type, the type ought to be returned as E.g. meta.aspect.type=core as discussed in #2620.

Although I want to change the name of aspect... filters is probably best.

{
    settings: [{...}, {...}],
    meta: {
        filters: { type: core }
    }
}

@jgable
Copy link
Contributor

jgable commented Apr 28, 2014

I worked through this yesterday and am just finishing up getting the tests to run now. The admin/settings pages look like they are working and all responses are now in the { settings: [{...}, {...}] }

@ErisDS
Copy link
Member Author

ErisDS commented Apr 28, 2014

👍

jgable added a commit to jgable/Ghost that referenced this issue May 1, 2014
Closes TryGhost#2606

- Refactor settings api responses to { settings: [ ] } format
- Update all code using api.settings to handle new response format
- Update test stubs to return new format
- Update client site settings model to parse new format into one object of key/value pairs
- Refactor to include all setting values
- Remove unused settingsCollection method
- Update settingsCache to store all attributes
- Update settingsResult to send all attributes
- Remove unnecessary when() wraps
- Reject if editing a setting that doesn't exist
- Reject earlier if setting key is empty
- Update tests with new error messages
- Use setting.add instead of edit that was incorrectly adding
- Update importer to properly import activePlugins and installedPlugins
- Update expected setting result fields
- Fix a weird situation where hasOwnProperty didn't exist 🤷
halfdan added a commit to halfdan/Ghost that referenced this issue May 6, 2014
refs TryGhost#2606
- Use new API format when updating settings from the client side
- Add additional test to test new API format
- Adjust functional tests to work with the new format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants