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

Some currently existing issues in staff-api src #80

Closed
14 of 18 tasks
dukenguyenxyz opened this issue Jul 14, 2021 · 2 comments
Closed
14 of 18 tasks

Some currently existing issues in staff-api src #80

dukenguyenxyz opened this issue Jul 14, 2021 · 2 comments
Assignees
Labels
type: bug something isn't working type: discussion Issue that can be resolved with discussion

Comments

@dukenguyenxyz
Copy link
Contributor

dukenguyenxyz commented Jul 14, 2021

It'd be quite annoying to open each issue for each of these dot points, so instead of doing so, I've created this issue which is comprised of all of the problems which I have found currently with staff-api.

For each dot point, only one reference example is included, please fix all other existing similar problems

Style

  • Prefer unless over if !(expression), example

Ameba did not run here probably due to it not going into the deeper macro context

Refactor

  • Store ENVS as constants in a single file, example: 1

Security

  • User var() for query sanitisation like here instead of this

Optimisation

  • User find_or_create instead of this

Mass Assignment

Prevent mass assignment of fields like id, and implement direct deserialisation, i.e. create_from_json

examples

For this example, I'm not sure why we are serialising credentials here though

Double serialisation

examples

#as_json for selective serialisation

Implement #as_json or expand [Clear JSONDeserialise model to accept annotation as macro arg in

examples

@dukenguyenxyz dukenguyenxyz added the type: bug something isn't working label Jul 14, 2021
@caspiano
Copy link
Contributor

TBH, definining to_json(json : JSON::Builder) would be the best way to go instead of to_h/as_json

It might be helpful to get some ideas from how active-model does it.
https://github.com/spider-gazelle/active-model/blob/master/src/active-model/model.cr#L62-L84

@jeremyw24 jeremyw24 assigned jeremyw24 and unassigned tassja Aug 2, 2021
@jeremyw24 jeremyw24 added the type: discussion Issue that can be resolved with discussion label Oct 11, 2021
@jeremyw24 jeremyw24 assigned stakach and tassja and unassigned jeremyw24 Oct 11, 2021
@tassja
Copy link
Contributor

tassja commented Feb 14, 2022

All points addressed except for as_json implementation which has been extracted into its own issue
#154

@tassja tassja closed this as completed Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug something isn't working type: discussion Issue that can be resolved with discussion
Projects
None yet
Development

No branches or pull requests

5 participants