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

Add user settings API + Logic #58

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

louneskmt
Copy link
Contributor

@louneskmt louneskmt commented Nov 1, 2020

This PR adds the routes and the logic to allow custom user settings.

New routes:

  • GET /v1/account/settings: get existing user settings
  • POST /v1/account/settings (request body: setting and value): update one setting to a new value, add it if needed

Related PR:

EddWills95
EddWills95 previously approved these changes Nov 2, 2020
Copy link
Member

@lukechilds lukechilds left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Really sorry for the delay in review.

I think this can be simplified quite a bit by using a nested object in user.json for settings instead of creating a new file. See feedback for detailed explanation.

Please let me know if I've overlooked something and there's a reason you think it's better to do it this way.

logic/auth.js Outdated Show resolved Hide resolved
routes/v1/account.js Outdated Show resolved Hide resolved
logic/auth.js Outdated Show resolved Hide resolved
@louneskmt louneskmt requested a review from lukechilds November 6, 2020 12:11
Copy link
Member

@lukechilds lukechilds left a comment

Choose a reason for hiding this comment

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

Just a few little things I noticed.

logic/auth.js Outdated Show resolved Hide resolved
logic/auth.js Outdated Show resolved Hide resolved
logic/auth.js Outdated Show resolved Hide resolved
logic/auth.js Outdated Show resolved Hide resolved
@bezysoftware
Copy link

This PR seems to be blocking couple of others, is there anything else that needs to be done?

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.

4 participants