GitHub repository input component #8

bartaz · 2016-11-24T10:23:52Z

DONE:

add simple component
basic input parsing (to get user/repo pair and repo URL)
talk to GH API to verify repo existence
move component state to redux store
connect store to component, remove component state
tests of the component
talk to GH API to verify snapcraft.yaml existence (can be one step with checking repo on GH?)

TODO (postponed to separate branch)

better styles / reuse form components from MU

bartaz · 2016-11-29T09:22:25Z

src/common/actions/repository-input.js

+        .then(() => dispatch(verifyGitHubRepositorySuccess(`https://github.com/${repo}.git`)))
+        .catch(error => dispatch(verifyGitHubRepositoryError(error)));
+    } else {
+      // TODO: above we return promise, here nothing - inconsistent and harder to test


Not sure how to best handle this.

The problem is that verifyGitHubRepository does 2 steps of verification.
One is synchronous (parse the input with parseGitHubUrl and check if it's valid GH repo URL) and the other is asynchronous (call GH API for snapcraft.yaml).

In async branch we return the fetch promise (so it can be tested). The sync branch just dispatches failure action immediately. This means both branches need different types of tests and the return value of this function is inconsistent.

I'm not sure what would be the best way to solve it? Wrapping sync branch in immediately resolved Promise works, but feels unnecessary and bit dirty. Maybe splitting that into 2 separate actions (one sync for just parsing the input and the other for async calling API)? Any other ideas?

Maybe redux-batched-actions? reduxjs/redux#911 (comment) & https://github.com/tshelburne/redux-batched-actions

Aside from the batched actions suggestion (which I've never used) it looks like it should be 2 distinct actions, from the perspective of a reviewer ;)

Yes, it seems it will be cleaner and easier to test to have them as separate actions.

Agree, validity of input should be checked in a different action. This could be dispatched by the changeRepositoryInput action creator.

bartaz · 2016-11-29T09:26:05Z

src/common/components/repository-input/index.js

+    super(props);
+  }
+
+  getStatusMessage() {


I'm bit inconsistent here.

At first I added statusMessage in reducer and updateStatusMessage 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?

Based on almost nothing but gut feeling, I would keep the string in the component.

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.

earnubs · 2016-11-29T10:04:31Z

src/common/actions/repository-input.js

+import 'isomorphic-fetch';
+import parseGitHubUrl from 'parse-github-url';
+
+export const CHANGE_REPOSITORY_INPUT = 'CHANGE_REPOSITORY_INPUT';


Consider dropping _INPUT, CHANGE_REPOSITORY (or SET_REPOSITORY?) is easier to understand, I think.

... or SET_GITHUB_REPO_URL ?

This particular action is triggered on every change of the input value (every key stroke), so maybe something like CHANGE_INPUT_VALUE or CHANGE_VALUE.

Current flow of actions is:

user types some value into input (we expect full URL or user/repo) - it just triggers the CHANGE_REPOSITORY_INPUT to store the user input.

user clicks the button - verifyGitHubRepository is called

user input is parsed (we get user/repo pair out of it if it's valid or null otherwise)

if we have valid user/repo pair we dispatch VERIFY_GITHUB_REPOSITORY (store user/repo pair in store) and ask GH API for snapcraft.yaml in this repo

if we get valid response we construct full repo URL and call _SUCCESS action to store it

in case of error we store the error with _ERROR action.

So SET_GITHUB_REPO_URL actually is something that happens in the very end after verification in _SUCCESS.

CHANGE_REPOSITORY_INPUT sounds alright to me, given that it's going to be called every time the value is changed.

Suggested alternatives: REPOSITORY_INPUT_ON_CHANGE, UPDATE_REPOSITORY_INPUT

The action is not to change the repository input, it's to change the repository url (or 'set', but my argument is against the word 'input', not the word 'change').

There's nothing about the action that's linked to inputs, or change events, or anything at a component level, naming the action with reference to inputs or events needlessly couples it to the component here.

@earnubs Ah - I see. CHANGE_REPOSITORY sounds fine in that case.

katiefenn · 2016-11-29T14:56:57Z

src/common/components/repository-input/index.js

+    } else if (input.success && input.repositoryUrl) {
+      message = `Repository ${input.repository} is valid.`;
+    } else if (input.errors) {
+      message = `Repository ${input.repository} is invalid.`;


I think we need to distinguish here between an invalid GitHub repository name ("081 811 8181" / "[email protected]") and an inaccessible / non-existant GitHub repository ("katiefenn/thisrepodoesntexist").

Suggested message: "Repository either does not exist, or access not authorised."

Yes, messages are not very helpful ATM. Moving sync parsing/validation to input update function separately from calling GH API should help with that.

katiefenn · 2016-11-29T15:06:53Z

src/common/components/repository-input/index.js

+  }
+
+  onButtonClick() {
+    this.props.dispatch(verifyGitHubRepository(this.props.repositoryInput.inputValue));


Is clicking the button the only way the user can initiate the verification of the GitHub repository? What happens when they enter a URL and hit enter?

It's a submit button, so I assumed it would work on enter press too. I'll have a look why it's not the case.

I think it's because there's no <form> element with an action or onSubmit handler.

katiefenn · 2016-11-29T15:09:05Z

src/common/components/repository-input/index.js

+  dispatch: PropTypes.func.isRequired
+};
+
+function mapStateToProps(state) {


This feels too low a level to bind components to state. This really should be the responsibility of the container instead of the component.

@katiefenn I just looked how we done it in MU and did the same. You mean moving the state/props binding and connect out of the component so that the binding would happen on a container lever (f.e. in home.js) and the container will just render the RepositoryInput component passing dispatch and rest of the needed props to it?

@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.

@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?

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?

That sounds like it'd work 👍

OK, let's do it in the next branch.

earnubs · 2016-11-30T09:46:11Z

src/common/actions/repository-input.js

+
+export function validateGitHubRepository(repository) {
+  return (dispatch) => {
+    const gitHubRepo = parseGitHubUrl(repository);


I wonder if this should be moved to the reducer? And that might help my argument on the number of and naming of actions to set the url?

…le on it

earnubs · 2016-11-30T11:42:21Z

src/common/components/repository-input/index.js

@@ -17,27 +17,29 @@ export class RepositoryInput extends Component {
    const input = this.props.repositoryInput;
    let message;

-    if (input.repository && input.isFetching) {
+    if (input.inputValue.length > 2 && !input.repository) {


minor ux nit, too early reporting of "invalid" input is annoying, is 2 the smallest this can be?

earnubs

lgtm

bartaz added the work in progress label Nov 24, 2016

bartaz changed the title ~~[WIP] First draft on GH repo component with simple URL parsing~~ GitHub repository input component Nov 24, 2016

bartaz force-pushed the github-repo-validation branch from 4207d60 to 1bf8c53 Compare November 24, 2016 12:40

bartaz added 3 commits November 25, 2016 16:17

First draft on GH repo component with simple URL parsing

c99e5e5

Added simple check of repository existence through GH API

1f787d1

Added redux actions and reducer for repository input

54b1733

bartaz force-pushed the github-repo-validation branch from 1bf8c53 to 54b1733 Compare November 25, 2016 16:30

bartaz added 2 commits November 29, 2016 10:05

Repository input component now keeps all the state in redux store

ff69127

Verify existence of snapcraft.yaml at GH

cb0064f

bartaz commented Nov 29, 2016

View reviewed changes

bartaz removed the work in progress label Nov 29, 2016

earnubs reviewed Nov 29, 2016

View reviewed changes

katiefenn reviewed Nov 29, 2016

View reviewed changes

bartaz added 3 commits November 29, 2016 17:28

Cleaning up repo input validation actions

cd13a57

Better validation messages, proper cleaning of errors in state

cb0192a

Make GH input a proper form so it can be submitted with Enter key

c36e2f0

earnubs reviewed Nov 30, 2016

View reviewed changes

bartaz added 2 commits November 30, 2016 12:33

Moved parsing GH URL from action to reducer to simplify actions

46f998b

Emoji, just to make status messages nicer before we put some real sty…

3985e70

…le on it

earnubs reviewed Nov 30, 2016

View reviewed changes

earnubs approved these changes Nov 30, 2016

View reviewed changes

bartaz merged commit 3c26cbb into master Nov 30, 2016

bartaz deleted the github-repo-validation branch December 1, 2016 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub repository input component #8

GitHub repository input component #8

bartaz commented Nov 24, 2016 •

edited

Loading

bartaz Nov 29, 2016

earnubs Nov 29, 2016

earnubs Nov 29, 2016

bartaz Nov 29, 2016

katiefenn Nov 29, 2016 •

edited

Loading

bartaz Nov 29, 2016

earnubs Nov 29, 2016

katiefenn Nov 29, 2016

earnubs Nov 29, 2016

earnubs Nov 29, 2016

bartaz Nov 29, 2016

katiefenn Nov 29, 2016

earnubs Nov 29, 2016 •

edited

Loading

earnubs Nov 29, 2016

katiefenn Nov 30, 2016

katiefenn Nov 29, 2016

bartaz Nov 29, 2016

katiefenn Nov 29, 2016

bartaz Nov 29, 2016

katiefenn Nov 29, 2016

katiefenn Nov 29, 2016 •

edited

Loading

bartaz Nov 29, 2016

katiefenn Nov 29, 2016

bartaz Nov 29, 2016

katiefenn Nov 29, 2016

bartaz Nov 29, 2016

earnubs Nov 30, 2016

earnubs Nov 30, 2016

earnubs left a comment

GitHub repository input component #8

GitHub repository input component #8

Conversation

bartaz commented Nov 24, 2016 • edited Loading

DONE:

TODO (postponed to separate branch)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katiefenn Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earnubs Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katiefenn Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earnubs left a comment

Choose a reason for hiding this comment

bartaz commented Nov 24, 2016 •

edited

Loading

katiefenn Nov 29, 2016 •

edited

Loading

earnubs Nov 29, 2016 •

edited

Loading

katiefenn Nov 29, 2016 •

edited

Loading