-
Notifications
You must be signed in to change notification settings - Fork 26
GitHub repository input component #8
Changes from 9 commits
c99e5e5
1f787d1
54b1733
ff69127
cb0064f
cd13a57
cb0192a
c36e2f0
46f998b
3985e70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import 'isomorphic-fetch'; | ||
|
||
export const SET_GITHUB_REPOSITORY = 'SET_GITHUB_REPOSITORY'; | ||
export const VERIFY_GITHUB_REPOSITORY = 'VERIFY_GITHUB_REPOSITORY'; | ||
export const VERIFY_GITHUB_REPOSITORY_SUCCESS = 'VERIFY_GITHUB_REPOSITORY_SUCCESS'; | ||
export const VERIFY_GITHUB_REPOSITORY_ERROR = 'VERIFY_GITHUB_REPOSITORY_ERROR'; | ||
|
||
export function setGitHubRepository(value) { | ||
return { | ||
type: SET_GITHUB_REPOSITORY, | ||
payload: value | ||
}; | ||
} | ||
|
||
function checkStatus(response) { | ||
if (response.status >= 200 && response.status < 300) { | ||
return response; | ||
} else { | ||
const error = new Error(response.statusText); | ||
error.response = response; | ||
throw error; | ||
} | ||
} | ||
|
||
export function verifyGitHubRepository(repository) { | ||
return (dispatch) => { | ||
if (repository) { | ||
dispatch({ | ||
type: VERIFY_GITHUB_REPOSITORY, | ||
payload: repository | ||
}); | ||
|
||
return fetch(`https://api.github.com/repos/${repository}/contents/snapcraft.yaml`) | ||
.then(checkStatus) | ||
.then(() => dispatch(verifyGitHubRepositorySuccess(`https://github.com/${repository}.git`))) | ||
.catch(error => dispatch(verifyGitHubRepositoryError(error))); | ||
} | ||
}; | ||
} | ||
|
||
export function verifyGitHubRepositorySuccess(repositoryUrl) { | ||
return { | ||
type: VERIFY_GITHUB_REPOSITORY_SUCCESS, | ||
payload: repositoryUrl | ||
}; | ||
} | ||
|
||
export function verifyGitHubRepositoryError(error) { | ||
return { | ||
type: VERIFY_GITHUB_REPOSITORY_ERROR, | ||
payload: error, | ||
error: true | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import 'isomorphic-fetch'; | ||
|
||
import React, { Component, PropTypes } from 'react'; | ||
import { connect } from 'react-redux'; | ||
|
||
import { | ||
setGitHubRepository, | ||
verifyGitHubRepository | ||
} from '../../actions/repository-input'; | ||
|
||
export class RepositoryInput extends Component { | ||
constructor(props) { | ||
super(props); | ||
} | ||
|
||
getStatusMessage() { | ||
const input = this.props.repositoryInput; | ||
let message; | ||
|
||
if (input.inputValue.length > 2 && !input.repository) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor ux nit, too early reporting of "invalid" input is annoying, is 2 the smallest this can be? |
||
message = 'Repository URL or name is invalid.'; | ||
} else if (input.repository && input.isFetching) { | ||
message = `Verifying ${input.repository} on GitHub...`; | ||
} else if (input.success && input.repositoryUrl) { | ||
message = `Repository ${input.repository} contains snapcraft project and can be built.`; | ||
} else if (input.error) { | ||
if (input.repository) { | ||
message = `Repository ${input.repository} is doesn't exist, is not public or doesn't contain snapcraft.yaml file.`; | ||
} | ||
} | ||
|
||
return message; | ||
} | ||
|
||
render() { | ||
const isValid = !!this.props.repositoryInput.repository; | ||
|
||
return ( | ||
<form onSubmit={this.onSubmit.bind(this)}> | ||
<label>Repository URL:</label> | ||
<input type='text' value={this.props.repositoryInput.inputValue} onChange={this.onInputChange.bind(this)} /> | ||
<button type='submit' disabled={!isValid}>Verify</button> | ||
<div> | ||
{this.getStatusMessage()} | ||
</div> | ||
</form> | ||
); | ||
} | ||
|
||
onInputChange(event) { | ||
this.props.dispatch(setGitHubRepository(event.target.value)); | ||
} | ||
|
||
onSubmit(event) { | ||
const { repository } = this.props.repositoryInput; | ||
|
||
if (repository) { | ||
this.props.dispatch(verifyGitHubRepository(repository)); | ||
} | ||
event.preventDefault(); | ||
} | ||
} | ||
|
||
RepositoryInput.propTypes = { | ||
repositoryInput: PropTypes.object.isRequired, | ||
dispatch: PropTypes.func.isRequired | ||
}; | ||
|
||
function mapStateToProps(state) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels too low a level to bind components to state. This really should be the responsibility of the container instead of the component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @katiefenn I just looked how we done it in MU and did the same. You mean moving the state/props binding and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bartaz: Maybe "container" wasn't the right word in this case. I think presentational components (such as RepositoryInput) should contain logic only for changing their appearance and defer responding to user input to their parent component. This article sums up what I mean ("presentational components have no dependencies on the rest of the app, such as Flux actions or stores"). In this case, I think it would be better to pass functions in as props like we do with the InputField component in JavanRhino. I think there's a value in all our form components behaving in similar ways and having similar interfaces. It would also allow us to have two instances of RepositoryInput on the same page, which is an unlikely situation but could prevent some tricky bugs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @katiefenn at this stage making RepositoryInput just a presentational component would mean more or less that it's just an input with a button and all of the other logic is moved out of it so some other layer that actually does data binding, updates state on keypress etc. I understand the point of doing it this way, but seems a little overkill on such a small thing this component is ATM. Maybe it will all make more sense once we take form components from MU and use it here for styling. This will mean that components like InputField, etc will be the dumb presentational components, while RepositoryInput will become the binding between the redux state and props of form components? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds like it'd work 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, let's do it in the next branch. |
||
const { | ||
repositoryInput | ||
} = state; | ||
|
||
return { | ||
repositoryInput | ||
}; | ||
} | ||
|
||
export default connect(mapStateToProps)(RepositoryInput); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import * as ActionTypes from '../actions/repository-input'; | ||
import parseGitHubUrl from 'parse-github-url'; | ||
|
||
function parseRepository(input) { | ||
const gitHubRepo = parseGitHubUrl(input); | ||
return gitHubRepo ? gitHubRepo.repo : null; | ||
} | ||
|
||
export function repositoryInput(state = { | ||
isFetching: false, | ||
inputValue: '', | ||
repository: null, | ||
repositoryUrl: null, | ||
success: false, | ||
error: false | ||
}, action) { | ||
switch(action.type) { | ||
case ActionTypes.SET_GITHUB_REPOSITORY: | ||
return { | ||
...state, | ||
inputValue: action.payload, | ||
repository: parseRepository(action.payload), | ||
success: false, | ||
error: false | ||
}; | ||
case ActionTypes.VERIFY_GITHUB_REPOSITORY: | ||
return { | ||
...state, | ||
isFetching: true | ||
}; | ||
case ActionTypes.VERIFY_GITHUB_REPOSITORY_SUCCESS: | ||
return { | ||
...state, | ||
isFetching: false, | ||
success: true, | ||
error: false, | ||
repositoryUrl: action.payload | ||
}; | ||
case ActionTypes.VERIFY_GITHUB_REPOSITORY_ERROR: | ||
return { | ||
...state, | ||
isFetching: false, | ||
success: false, | ||
error: action.payload | ||
}; | ||
default: | ||
return state; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import expect from 'expect'; | ||
|
||
// Custom assertions | ||
expect.extend({ | ||
toHaveActionOfType(expected) { | ||
expect.assert( | ||
this.actual.filter((action) => { | ||
return action.type === expected; | ||
}).length > 0, | ||
'to have action %s', | ||
expected | ||
); | ||
|
||
return this; | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import './helpers'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
import expect from 'expect'; | ||
import configureMockStore from 'redux-mock-store'; | ||
import thunk from 'redux-thunk'; | ||
import nock from 'nock'; | ||
import { isFSA } from 'flux-standard-action'; | ||
|
||
import { | ||
setGitHubRepository, | ||
verifyGitHubRepository, | ||
verifyGitHubRepositorySuccess, | ||
verifyGitHubRepositoryError | ||
} from '../../../../../src/common/actions/repository-input'; | ||
import * as ActionTypes from '../../../../../src/common/actions/repository-input'; | ||
|
||
const middlewares = [ thunk ]; | ||
const mockStore = configureMockStore(middlewares); | ||
|
||
describe('repository input actions', () => { | ||
const initialState = { | ||
isFetching: false, | ||
inputValue: '', | ||
repository: null, | ||
repositoryUrl: null, | ||
statusMessage: '', | ||
success: false, | ||
error: false | ||
}; | ||
|
||
let store; | ||
let action; | ||
|
||
beforeEach(() => { | ||
store = mockStore(initialState); | ||
}); | ||
|
||
context('setGitHubRepository', () => { | ||
let payload = 'foo/bar'; | ||
|
||
beforeEach(() => { | ||
action = setGitHubRepository(payload); | ||
}); | ||
|
||
it('should create an action to update repository name', () => { | ||
const expectedAction = { | ||
type: ActionTypes.SET_GITHUB_REPOSITORY, | ||
payload | ||
}; | ||
|
||
store.dispatch(action); | ||
expect(store.getActions()).toInclude(expectedAction); | ||
}); | ||
|
||
it('should create a valid flux standard action', () => { | ||
expect(isFSA(action)).toBe(true); | ||
}); | ||
}); | ||
|
||
context('verifyGitHubRepositorySuccess', () => { | ||
let payload = 'http://github.com/foo/bar.git'; | ||
|
||
beforeEach(() => { | ||
action = verifyGitHubRepositorySuccess(payload); | ||
}); | ||
|
||
it('should create an action to save github repo url on success', () => { | ||
const expectedAction = { | ||
type: ActionTypes.VERIFY_GITHUB_REPOSITORY_SUCCESS, | ||
payload | ||
}; | ||
|
||
store.dispatch(action); | ||
expect(store.getActions()).toInclude(expectedAction); | ||
}); | ||
|
||
it('should create a valid flux standard action', () => { | ||
expect(isFSA(action)).toBe(true); | ||
}); | ||
}); | ||
|
||
context('verifyGitHubRepositoryError', () => { | ||
let payload = 'Something went wrong!'; | ||
|
||
beforeEach(() => { | ||
action = verifyGitHubRepositoryError(payload); | ||
}); | ||
|
||
it('should create an action to store github repo error on failure', () => { | ||
const expectedAction = { | ||
type: ActionTypes.VERIFY_GITHUB_REPOSITORY_ERROR, | ||
error: true, | ||
payload | ||
}; | ||
|
||
store.dispatch(action); | ||
expect(store.getActions()).toInclude(expectedAction); | ||
}); | ||
|
||
it('should create a valid flux standard action', () => { | ||
expect(isFSA(action)).toBe(true); | ||
}); | ||
}); | ||
|
||
context('verifyGitHubRepository', () => { | ||
let scope; | ||
|
||
beforeEach(() => { | ||
scope = nock('https://api.github.com'); | ||
}); | ||
|
||
afterEach(() => { | ||
nock.cleanAll(); | ||
}); | ||
|
||
it('should save GitHub repo on successful verification', () => { | ||
scope.get('/repos/foo/bar/contents/snapcraft.yaml') | ||
.reply(200, { | ||
'name': 'snapcraft.yaml' | ||
}); | ||
|
||
return store.dispatch(verifyGitHubRepository('foo/bar')) | ||
.then(() => { | ||
expect(store.getActions()).toHaveActionOfType( | ||
ActionTypes.VERIFY_GITHUB_REPOSITORY_SUCCESS | ||
); | ||
}); | ||
}); | ||
|
||
it('should store error on GitHub verification failure', () => { | ||
scope.get('/repos/foo/bar/contents/snapcraft.yaml') | ||
.reply(404, { | ||
'message': 'Not Found', | ||
'documentation_url': 'https://developer.github.com/v3' | ||
}); | ||
|
||
return store.dispatch(verifyGitHubRepository('foo/bar')) | ||
.then(() => { | ||
expect(store.getActions()).toHaveActionOfType( | ||
ActionTypes.VERIFY_GITHUB_REPOSITORY_ERROR | ||
); | ||
}); | ||
}); | ||
|
||
}); | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm bit inconsistent here.
At first I added
statusMessage
in reducer andupdateStatusMessage
action to keep the validation/status message in the store too. But once I got to that it seemed more natural to construct status message in the component based on the state from the store.Not sure which way to follow? Keep messages in the component (view) and remove it from reducer/action? Or move messages to the store (so the actions themselves will update the messages in the store and component will just display it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on almost nothing but gut feeling, I would keep the string in the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good example to discuss the limits of presentation component logic. I think this amount of switching is okay, but if it became any more complex it would be better to move it into the store.
From a testing perspective, it's nice to pass props like "message" and "status" to presentational components and test whether they are used. But I'm borderline in this instance.