oarevalo / BugLogHQ

BugLogHQ is a tool to centralize the handling of automated bug reports from multiple applications. BugLogHQ provides a unified view of error messages sent from any number of applications, allowing the developer to search, graph, forward, and explore the bug reports submitted by the applications.
http://www.bugloghq.com
155 stars 67 forks source link

CORS (rev1) #130

Closed coryasilva closed 8 years ago

coryasilva commented 8 years ago

@oarevalo This is working in our simple production environment and everything works. Though, please review the code as I am a coldfusion noob.

This is in response to pull request #128, but with proper branching

oarevalo commented 8 years ago

Looks pretty good, the only change I think would make sense is to move the value of Access-Control-Max-Age to the config. Seems a bit of a "magic number" to just include it directly on the template. Also would make it for easier customization.

coryasilva commented 8 years ago

Yeah I was thinking about moving all of the header values to the settings but was trying not to pollute the settings and/or overwhelm the users. Then again if you are tracking bugs you are probably a developer or IT guy...

What do you think all settings or just add maxAge

oarevalo commented 8 years ago

I had the same thought (re: too many settings). But there is a trick that can allow conciseness and also extensibility. The getSetting() method takes a second argument for a default value, so that if the requested setting is not found on the Config file, it returns that value.

That way you can do something like:

var cors.allowCredentials = service.getConfig().getSetting("cors.allowCredentials", false);

without having to necessarily add more stuff to the config. The only important bit would be to add some documentation somewhere indicating the additional/optional config settings that can be used to configure CORS. I think just a brief comment on the listener cfm indicating the names of the used settings should be enough.

coryasilva commented 8 years ago

Okay that makes sense; I think my last commit should do it.

oarevalo commented 8 years ago

Merged to the 1.8 branch. Will be merged into master shortly.

coryasilva commented 8 years ago

Awesome. Thanks.