search

empowerai / fs-permit-platform

Module for intake of special use applications for Forest Service Application Permits
Other
3 stars 0 forks source link

As a user, I need the server code to be reliable and maintainable so that I can use the site #554

Open shekarpendem opened 6 years ago

shekarpendem commented 6 years ago

Notes

Acceptance criteria

Tasks

Deployment and continuous integration

Server

DBA

API docs

SRC

Auth
Controllers
Services

Note: I like how most of these methods have jsDoc comments

Email

Tests

VCAP Services

Definition of Done

sadlerw commented 6 years ago

@lauraGgit to address the "quotation of the cutting area keys" question - I can't find your question, but I can say that we are inserting JSON into the db there so it has to be valid json, meaning it has to have quoted keys within the string of JSON going into the field. Maybe get us in a zoom to discuss if that isn't it please?

sadlerw commented 6 years ago

@lauraGgit re: refactoring the intake client base URL I would think something like this would have its own environment variable - INTAKE_CLIENT_BASE_URL for example Is that preferable since it really isn't a credential?

sadlerw commented 6 years ago

for 'idp.int.login.gov' this entry: vcapConstants.LOGIN_GOV_HOST = (typeof process.env.LOGIN_GOV_HOST === undefined ? 'idp.int.login.gov' : process.env.LOGIN_GOV_HOST); will use that as default unless LOGIN_GOV_HOST is defined

lauraGgit commented 6 years ago

@sadlerw Per the quotations - there are a number places where the cutting areas json has a json on the front end that seems to have keys that are not single quoted- you might want to take a look at the Sprint 10 pr comments for this. It seems both inconsistent from the frontend to the server and a bit confusing.

lauraGgit commented 6 years ago

@sadlerw INTAKE_CLIENT_BASE_URL yes, if we have a node/ env file now for the server we can just take it out of the vcap and have it as a checked in env var that is still environment dependent. We would also need to update the manifests.

lauraGgit commented 6 years ago

I'd rather not have the default login host be hardcoded at all, because if its not defined and PLATFORM != local thank the build should fail

sadlerw commented 6 years ago

Re: quotation the standards for json is quoted keys but they are optional in typescript and es6. They are not optional in a string being inserted into the db. They are optional in coffeescript for example, but not javascript before es5 (I think). Linting standards to make all json representations in the entire codebase uniform are not desirable.

sadlerw commented 6 years ago

OK we'll coordinate with you on the deploy to set both INTAKE_CLIENT_BASE_URL and LOGIN_GOV_HOST as environment variables. Staging will fail to start until these are configured. We will add them to our circle config too. Maybe sometime after sprint retro.

sadlerw commented 6 years ago

on the vcap changes - remove hardcoding of HOST in login gov auth in header and replace with the host from the discovery url?

on the email templates - I don't see an easy way to do this. Please open a new story specifically for this task if you think it should happen still

cameronwolf commented 6 years ago

FYI for the task to look at extracting dependencies from docker and cache them, in theory it is possible. Upon trying to implement there was an error when circle tried to extract the cache and errorred out. This appears to be a known issue with no current solution. I was able to update the step which had installed all server dependencies so that only snyk was installed before the deploy job.