Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

Add configuration/settings endpoints #515

Merged
merged 59 commits into from
Nov 20, 2018
Merged

Conversation

T-Dnzt
Copy link

@T-Dnzt T-Dnzt commented Nov 9, 2018

Issue/Task Number: #502
closes #502

Overview

This PR adds two new endpoints to manage the configuration settings:

  • /configuration.get
  • /configuration.update

I decided to go with configuration.* instead of settings. simply because we already have a /settings.all endpoint that returns something in the shape of

%{
  minted_tokens: []
}

I didn't know how to reuse that and make it fit with the settings system, so using something like /configuration.* made more sense. Totally up for discussion if you guys have better ideas.

Changes

  • Add count to the pagination object to know exactly how many records are being returned in the response (suggested by one of our mobile developers, a small change that makes it easier to parse a response and adds little overhead in the code).
  • Add /configuration.get to retrieve the list of all settings. This actually returns a paginated, filterable and sortable list of settings. /configuration.all could be a better name to stay consistent.
  • Add /configuration.update that takes a hash of settings update in the form of:
%{
  "setting_name_1": "setting_value_1",
  "setting_name_2": "setting_value_2",
}

And will return a list of insert results which can either be:

  • an updated setting (configuration_setting type)
  • an error changeset (error type)
  • a configuration_setting_error (that would occur in case a setting key is not found)

To Do

  • Update Swagger

@T-Dnzt T-Dnzt requested a review from unnawut November 9, 2018 09:30
@T-Dnzt T-Dnzt requested a review from mederic-p November 9, 2018 09:30
@T-Dnzt T-Dnzt added this to the v1.1 milestone Nov 9, 2018
use AdminAPI.ConnCase, async: true

describe "/configuration.get" do
test "returns a list of accounts and pagination data" do
Copy link
Contributor

Choose a reason for hiding this comment

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

list of configurations

assert data["fake_setting"] == %{
"code" => "setting_not_found",
"key" => "fake_setting",
"object" => "configuration_setting_error"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to have 2 different kind of error objects returned? If possible I'd stick with only error and try to remove configuration_setting_error

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@T-Dnzt T-Dnzt changed the base branch from 436-settings-system to master November 14, 2018 10:52
######################################
SettingResponse:
description: "The response schema for settings"
SettingSchema:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're using "configuration" now, should this be changed to ConfigurationSchema?

Also SettingResponse -> ConfigurationResponse a bit down below.

Copy link
Author

Choose a reason for hiding this comment

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

I wish it was just settings, but it was taken, so instead we send back a configuration composed of a set of settings, that's why you see both settings and configuration in the spec depending on the level.

@unnawut unnawut removed their assignment Nov 19, 2018
@mederic-p mederic-p removed their assignment Nov 20, 2018
@T-Dnzt T-Dnzt merged commit 2629d80 into master Nov 20, 2018
@ghost ghost removed the s3/review 👀 label Nov 20, 2018
@T-Dnzt T-Dnzt deleted the 502-add-settings-endpoints branch November 20, 2018 05:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow settings to be updated through Admin API endpoints
3 participants