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

User middleware #660

Merged
merged 43 commits into from
Jan 24, 2017
Merged

User middleware #660

merged 43 commits into from
Jan 24, 2017

Conversation

bergie
Copy link
Member

@bergie bergie commented Jan 20, 2017

Moves user data handling from mixture of storage and UI to a Redux-style middleware graph:

screenshot 2017-01-23 at 17 39 17

Middleware components receive an action, and can either pass the action onwards, or capture it and emit their own actions instead.

This way for instance a user:login action gets caught by the UserMiddleware graph, which then performs the OAuth dance, and down the line emits either user:info (for successful login) or user:error (for failed login).

@bergie bergie changed the title WIP: User middleware User middleware Jan 23, 2017
@bergie bergie requested a review from jonnor January 23, 2017 16:51
async: true
, (data, groups, out, callback) ->
actionParts = c.params.action.split ':'
setTimeout ->
Copy link
Member

Choose a reason for hiding this comment

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

Why setTimeout?

Copy link
Member Author

Choose a reason for hiding this comment

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

Workaround for bracket forwarding bug in WirePattern. Would be fixed by going 0.8 process API

c.inPorts.add 'action',
datatype: 'string'
required: true
control: true
Copy link
Member

Choose a reason for hiding this comment

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

Why control: true ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really needed, was going to do it with process API first, but decided to stick to WirePattern until 0.8 stable is out

async: true
forwardGroups: false
, (data, groups, out, callback) ->
# This will in effect cause a NoFlo network stop as the app
Copy link
Member

@jonnor jonnor Jan 23, 2017

Choose a reason for hiding this comment

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

Can't we use some single-page-app magic to avoid redirect reloading page? For future..

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using this to redirect to GitHub, too. The URL query cleanup we do post-login can be done down the line with pushState

<script src="CreateProject.js"></script>
<script src="EditGraph.js"></script>
<script src="DeleteGraph.js"></script>
<script src="EndToEndInitialization.js"></script>
Copy link
Member

@jonnor jonnor Jan 23, 2017

Choose a reason for hiding this comment

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

Can we use a directory for these tests? spec/endtoend/Login.coffee or spec/acceptance/Login.coffee, or spec/ui/...

"fbp": "~1.5.0",
"fbp-spec": "~0.1.15",
"flowhub-registry": "^0.1.0",
"font-awesome": "^4.6.3",
"indexeddbshim": "^2.2.1",
"jshint": "^2.9.2",
"microflo": "~0.3.43",
"noflo": "~0.7.9",
"noflo": "^0.7.9",
Copy link
Member

Choose a reason for hiding this comment

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

On the 0.x series these are effectively the same in semver. Why the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think side effect of trying 0.8 beta and then going back to 0.7 to get only one NoFlo into the bundle (as opposed to N pulled in via individual modules)

@@ -46,5 +46,3 @@ exports.getComponent = ->
return callback err if err
out.send valid
do callback

c
Copy link
Member

Choose a reason for hiding this comment

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

Bug? Or does WirePattern return the component?

Copy link
Member Author

Choose a reason for hiding this comment

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

WirePattern returns the component, just like most of the component methods

it 'should pass it out as-is', (done) ->
action = 'runtime:connect'
payload =
hello: 'world'
Copy link
Member

@jonnor jonnor Jan 23, 2017

Choose a reason for hiding this comment

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

Eh, what is this case for? Does not seem useful. Please use test-data which is representative for real usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted a test of a passed packet. The contents don't really matter as long as:

  • the action is one that the middleware isn't supposed to handle
  • we verify the packet gets passed through without manipulation (strict equality check)

else
baseDir = 'noflo-ui'

describe 'User Middleware', ->
Copy link
Member

Choose a reason for hiding this comment

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

Yay tests!

actionIn.port = 'action'
###
c.network.on 'begingroup', (data) ->
console.log " < #{data.id}"
Copy link
Member

Choose a reason for hiding this comment

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

Begging for flowtrace...

Copy link
Member Author

Choose a reason for hiding this comment

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

No flowtraces on bare (non-runtime) NoFlo networks I think? Would be super useful

token = 'niov2o3wnnv4ioufuhh92348fh42q9'
xhr = sinon.fakeServer.create()
afterEach ->
xhr.restore()
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to use a name for the mock which singifies testing more than xhr. Like mock or mockAuth

message: 'Bad Credentials'
do xhr.respond
do xhr.respond
describe 'without user and with valid grant code in URL', ->
Copy link
Member

Choose a reason for hiding this comment

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

Should this be with user instead of without?

@jonnor
Copy link
Member

jonnor commented Jan 23, 2017

No unused components to delete after this?

@jonnor
Copy link
Member

jonnor commented Jan 23, 2017

I think the action outputs on the middlewares should be something like new (or created or similar), to better clarify their semantics and match pass.

@jonnor
Copy link
Member

jonnor commented Jan 23, 2017

@bergie I don't understand how this is different from the existing "Store" code... I don't see any removed code from UI layer?

@bergie
Copy link
Member Author

bergie commented Jan 24, 2017

RemoteLogin and UserStorage components were removed in this PR (RemoteLogin was split to multiple components for better testability).

No code removal from UI yet, though now that we have all of this in place removing the user API calls in noflo-account-settings is a fast follow

@jonnor
Copy link
Member

jonnor commented Jan 24, 2017

@bergie ok looks good to me. Merge at will

@bergie bergie merged commit 1164d1d into master Jan 24, 2017
@bergie bergie deleted the user_middleware branch January 24, 2017 12:25
@bergie bergie mentioned this pull request Jan 30, 2017
2 tasks
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