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

Mnoe dev toolkit #261

Merged
merged 17 commits into from
Nov 25, 2016
Merged

Mnoe dev toolkit #261

merged 17 commits into from
Nov 25, 2016

Conversation

xaun
Copy link
Contributor

@xaun xaun commented Oct 20, 2016

@cesar-tonnoir please review - not ready for merge though. I need to update the developer docs, which I will do once @ouranos get's back to me about CORS so I can commit & document the uat urls in default settings. 🍻

@xaun
Copy link
Contributor Author

xaun commented Oct 20, 2016

screen shot 2016-10-20 at 18 19 40

screen shot 2016-10-20 at 18 21 58

Copy link
Contributor

@cesar-tonnoir cesar-tonnoir left a comment

Choose a reason for hiding this comment

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

Looks nice to me. I would just recommend to update routes.svc in the same PR

return _self._defaults = angular.copy(DEFAULTS);
}

// NOTE: this will be refactored a lot when mnoe routes are added as default
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed -> isn't it a good occasion to do it? :)
Looks like you've done 80% of the work below...

@xaun
Copy link
Contributor Author

xaun commented Nov 16, 2016

@ouranos please review the "Merge pull request #6 from xaun/remove-proxy" changes to see my working solution for providing CSRF support for the Dev Toolkit!

So you were right, Angular does not apply the CSRF token to CORS requests. Took a bit of work to figure out the best way to apply this, and once I did that, I realised that the CSRF token gets refreshed on various Angular Devise methods, so I had to come up with a little service to manage that.

@cesar-tonnoir FYI & not ready for merge.

Related PRs:
https://github.com/maestrano/impac/pull/379
https://github.com/maestrano/impac-express/pull/8

Remaining works:

  • Deploy CORS configurations in the related PRs and test via https.
  • Set the defaults urls to mnohub & impac uat.
  • Refactor the impac-angular routes svc
  • Update the developer.md & readme.md where needed

@ouranos
Copy link
Contributor

ouranos commented Nov 19, 2016

Reviewed: https://github.com/xaun/impac-angular/pull/6
LGTM. Just need to edit the host to point to express isn't it?

@cesar-tonnoir
Copy link
Contributor

@xaun is there anything preventing you from merging this PR?
If not can you please merge and update https://maestrano.atlassian.net/wiki/display/DEV/Temporary+workaround+for+the+Developer+Workspace (step 7.) once done ?

@xaun xaun changed the base branch from 1.4 to release/1.4.5 November 25, 2016 13:21
@xaun xaun dismissed cesar-tonnoir’s stale review November 25, 2016 13:21

Changes have now been approved @ouranos

@xaun xaun merged commit d1f4ebc into maestrano:release/1.4.5 Nov 25, 2016
@xaun xaun deleted the mnoe-dev-toolkit branch November 25, 2016 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants