Manage Experiments from the front #195

carlosms · 2018-03-02T14:57:41Z

Part of #160, this PR will add a new route to create & edit experiments, and a way to upload a new SQLite DB with file pairs.

smacker · 2018-03-02T15:36:57Z

src/pages/ExperimentsAdmin.js

+}
+
+const mapStateToProps = state => ({
+  user: state.user,


looks like you don't use it anywhere

smacker · 2018-03-02T15:37:46Z

src/state/experiments.js

 export const initialState = {
  loading: true,
  error: null,
+  createOnProgress: false,


shouldn't it be in progress?

smacker · 2018-03-02T15:44:29Z

src/pages/ExperimentsAdmin.js

+
+class ExperimentsAdmin extends Component {
+  componentDidMount() {
+    this.props.load();


no need to fix it, just to let you know. Here we have kind of "bug" here. Because user will come here from another page in most of the cases we will have experiments in the store already. So a user will see the list of experiments and in few milliseconds it will be replaced by a loader and then by list again.

This was actually copied from Experiments.js.
I guess one way to handle this would be to have an expiration date for the data, and then the function could load only if the list is empty, or the expiration date passed... But for now in my opinion a forced load is not that inconvenient for the user experience.

smacker · 2018-03-02T15:46:10Z

src/state/experiments.js

+  return api
+    .createExperiment(name, description)
+    .then(() => {
+      dispatch({ type: CREATE_SUCCESS });


no need to fix. Just let you know, an optimistic update would fit nicely here.

carlosms · 2018-03-09T18:42:07Z

This is how it looks, based on the wireframes provided by @ricardobaeta here #160 (comment), but keeping the name 'experiment' for consistency with the rest of the app.

dpordomingo · 2018-03-09T19:35:17Z

Conflicts appeared :(

smacker

some comments.
also tests are missed.

smacker · 2018-03-12T09:33:20Z

src/state/experiments.js

+  uploadResult: UPLOAD_RES_NONE,
+
+  editModalShow: false,
+  editModalExpID: null,


could you please keep names consistent with all other js files?
In js in general and in code-annotation frontend camelCase is a standard.

I must be missing something, sorry, can you paste me the exact change you have in mind?

editModalExpID -> editModalExpId. Just because it's IdnotID` everywhere.

smacker · 2018-03-12T09:36:25Z

src/state/experiments.js

+export const UPLOAD = 'ca/experiments/UPLOAD';
+export const UPLOAD_SUCCESS = 'ca/experiments/UPLOAD_SUCCESS';
+export const UPLOAD_ERROR = 'ca/experiments/UPLOAD_ERROR';
+export const OPEN_EDIT_MODAL = 'ca/experiments/OPEN_EDIT_MODAL';


usually, it's a good idea to keep store agnostic to presentation level.

To be honest I didn't like that much the previous code, but I didn't want to spend much more time refactoring it. I've tried to improve it in ce357ff, moving the modal state to Experiments.js

smacker · 2018-03-12T09:42:25Z

src/components/Experiments/ExperimentEditModal.js

+        this.state.nameVal,
+        this.state.descriptionVal
+      );
+    } else {


please follow early return rule as all other js code.

Changed in 0fb33e0

Can you please point me to a guideline (like the airbnb one we use in eslint.rb) where this rule is explained?

the rule usually you look how the current code is written and do the same.

there is a rule about no-else return in airbnb:
https://github.com/airbnb/javascript#blocks--no-else-return

no always early return because sometimes it makes sense to not return early (though it was there before)

you can read more why people do that by simple googling early return. It's very popular pattern in javascript. Java style of else everywhere is much less common.

Ok, thanks for clarifying. I'm aware of the rules you mention, but I just didn't see them applying here and wanted to check if you were talking about something else. From my point of view the code was:

if (condition) somehting() else other() end // Posibly some more code

Now I understand your point of view, as there was no more code after the else.

smacker · 2018-03-12T09:47:40Z

src/components/Experiments/ExperimentEditModal.js

+  }
+
+  componentWillReceiveProps(nextProps) {
+    const exp = nextProps.editModalExperiment;


I see this code 2 times. The copy-past itself is not a problem. But component depends on both constructor and componentWillReceiveProps. So any the change must be done in both places. It would be much better to extract this code to function which returns state. And do this.state = fn(props) in constructor and this.setState(fn(nextProps)) here.

Makes sense, it's an easy improvement. Changed in dd9e4fb

smacker · 2018-03-12T09:49:18Z

src/components/Experiments/ExperimentEditModal.js

+
+  componentWillReceiveProps(nextProps) {
+    const exp = nextProps.editModalExperiment;
+    if (exp !== undefined) {


the check should compare nextProps with current props. If you need more info, come over and I can explain it verbally.

Changed in dd9e4fb

smacker · 2018-03-12T09:50:58Z

src/components/Experiments/ExperimentEditModal.js

+
+    let uploadIcon;
+    let uploadClass;
+    if (uploadInProgress) {


looks like switch will be better fit here

Are you sure? The chained if's condition use two different variables, I don't think it will be cleaner with a switch.

and if → else switch would be clearer, but I'd not block this wonderful work for this cosmetic suggestion 😉

if (uploadInProgress) { /* something */ } else switch (uploadResult) { case UPLOAD_RES_SUCCESS: /* something */ break; case UPLOAD_RES_FAILURE: /* something */ break; default: /* something */ }

smacker · 2018-03-12T09:56:28Z

src/components/Experiments/ExperimentEditModal.js

+    }
+
+    return (
+      <Modal show={editModalShow} onHide={this.handleClose}>


I would recommend controlling show outside of the modal component. In general it's better to do editModalShow && <Modal>. Otherwise it might not render virtual/real dom if the underlying component is written well, but all life cycle methods would be executed on every parent update.
Not very important in this particular case though.

Makes sense. I decided to leave it as a property of the Modal component to avoid introducing more and more changes. Let me know if you think this should be changed considering the last changes.

smacker · 2018-03-12T10:02:03Z

src/state/experiments.js

+    .then(res => {
+      dispatch({ type: CREATE_SUCCESS });
+      dispatch(load());
+      dispatch(showEditModal(true, res.id));


here is a good example why it's better to keep store view agnostic. create action can be called from anywhere. But it will set the modal show to true. It is super confusing and can cause weird bugs.

Changed in ce357ff

ricardobaeta · 2018-03-12T14:55:34Z

Thank you @carlosms :) It looks great!

carlosms · 2018-03-12T14:59:36Z

@dpordomingo I rebased and there are no more conflicts (I was avoiding it to not loose the commit hashes).

@smacker feel free to give it another pass. I was delaying making the tests to wait for change requests, I'll upload them shortly.

smacker · 2018-03-12T15:35:37Z

src/state/experiments.js

 export const LOAD_SUCCESS = 'ca/experiments/LOAD_SUCCESS';
 export const LOAD_ERROR = 'ca/experiments/LOAD_ERROR';
 export const SET = 'ca/experiments/SET';
+export const CREATE = 'ca/experiments/CREATE';


it's not really wrong, but kind of weird. You can see how it's done in redux usually in official examples: https://github.com/reactjs/redux/tree/master/examples/todomvc/src

smacker · 2018-03-13T09:51:10Z

src/components/Experiments/ExperimentEditModal.js

+          this.props.onOpenEdit(expId);
+        });
+
+      return;


not required here, but usually it's a good idea when you have a promise to return it. For example, you would need it for tests.

smacker · 2018-03-13T09:53:00Z

src/components/Experiments/ExperimentEditModal.js

+    if (create) {
+      this.props
+        .experimentCreate(this.state.nameVal, this.state.descriptionVal)
+        .then(expId => {


I usually put such logic in dispatchToProps, to keep code of the component abstract. Not a big problem though.

smacker · 2018-03-13T09:56:14Z

src/state/__snapshots__/experiments.test.js.snap

+Object {
+  "createInProgress": false,
+  "error": "test error",
+  "initialState": Object {


it's incorrect.

Yeah, I missed the spread operator while c&p and of course the tests were not failing 😄

smacker · 2018-03-13T09:56:26Z

src/state/__snapshots__/experiments.test.js.snap

+exports[`experiments/reducer CREATE_SUCCESS 1`] = `
+Object {
+  "createInProgress": false,
+  "initialState": Object {


it's incorrect.

smacker · 2018-03-13T09:57:07Z

src/state/__snapshots__/experiments.test.js.snap

+exports[`experiments/reducer UPDATE_ERROR 1`] = `
+Object {
+  "error": "test error",
+  "initialState": Object {


it's incorrect.

smacker · 2018-03-13T09:57:19Z

src/state/__snapshots__/experiments.test.js.snap

+
+exports[`experiments/reducer UPDATE_SUCCESS 1`] = `
+Object {
+  "initialState": Object {


it's incorrect.

smacker · 2018-03-13T09:59:14Z

src/components/Experiments/ExperimentEditModal.js

+    if (create) {
+      this.props
+        .experimentCreate(this.state.nameVal, this.state.descriptionVal)
+        .then(expId => {


in case of error here will be undefined and this.props.onOpenEdit will fail

smacker

tests are incorrect
found 1 bug
few suggestions (not required, up to you)

smacker · 2018-03-14T11:08:16Z

src/state/experiments.test.js

+      const errText = 'some error';
+      fetch.mockReject(errText);
+
+      store.dispatch(create('new_name', 'new_desc')).catch(() => {


problem with this test: if the action doesn't throw any error, test still pass.

Fixed: 435f086

dpordomingo

Wow!!! it worked perfectly! Many many thanks!
I only detected some (imho) UX problems that should be confirmed by @ricardobaeta in a next iteration.

dpordomingo · 2018-03-14T16:20:57Z

src/components/Experiments/ExperimentEditModal.js

+              </p>
+            </Col>
+            <Col xs={5} className="text-right">
+              <FormGroup controlId="uploadTest">


If uploadDisabled is true because it is being created a new experiment, the upload button should not appear at all.

dpordomingo · 2018-03-14T16:23:56Z

src/components/ExperimentsList.js

+                  {Math.round(exp.progress)}%
+                </td>
+                <td className="experiments-list__additional-actions">
+                  {/*


It's in master, I just changed the indentation 🤷‍♂️

dpordomingo · 2018-03-14T16:34:40Z

src/components/Experiments/ExperimentEditModal.js

+      return this.props
+        .experimentCreate(this.state.nameVal, this.state.descriptionVal)
+        .then(expId => {
+          this.props.onOpenEdit(expId);


It is kind of weird that after creating an experiment, the same modal is opened again without giving any special feedback to the user.

After creating a new experiment, I wouldn't open the modal automatically (as in edition-mode), so the user should open it manually -> she/he will be then aware of the edit-state.

dpordomingo · 2018-03-14T16:37:31Z

src/components/Experiments/ExperimentEditModal.js

+      this.props.editModalExperiment.id,
+      this.state.nameVal,
+      this.state.descriptionVal
+    );


The save button should not open the same modal again without clear feedback.

If the update succeeds, it should be said so, and the modal should be closed

if the update fails, it should be shown an error

dpordomingo · 2018-03-14T16:38:14Z

src/components/Experiments/ExperimentEditModal.js

+              <FormGroup controlId="uploadTest">
+                <FormControl
+                  type="file"
+                  onChange={this.handleUpload}


I would not push/save the new DB till the user press "save".

dpordomingo · 2018-03-14T16:53:14Z

@carlosms you did a great job, many thanks.

Since most important requirements were already addressed, we can merge, so let's proceed as it follows:

you squash/rebase and whatever you think it should be done,
then you merge,
then I'll open an issue with mi UX concerns so @ricardobaeta will be able to address them,
and I'll open a new issue with the issue in one testcase as it was spotted by @smacker

Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Add api call & reducer for sqlite DB upload Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Rename OnProgress -> InProgress Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Update experiments test snapshot Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Remove /experiments route, move btns to /dashboard Adds tmp buttons to the dashbard to test the API Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> New modal component to create new experiments Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Add experiment update action to the frontend Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Add the upload DB option to the modal Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Add styling Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Refactor ExperimentEditModal.handleSave Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Refactor ExperimentEditModal state from props Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Refactor experiments modal state out of the store Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Rename ID -> Id for consistency Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Update tests Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Fix bug in tests Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Fix bug when create action fails Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com> Add expect.hasAssertions to tests Signed-off-by: Carlos Martín <carlos.martin.sanchez@gmail.com>

smacker mentioned this pull request Mar 2, 2018

Experiments management #160

Closed

5 tasks

carlosms force-pushed the issue-173 branch from a5eeb0c to 5b3a143 Compare March 2, 2018 15:02

smacker reviewed Mar 2, 2018

View reviewed changes

smacker suggested changes Mar 12, 2018

View reviewed changes

carlosms force-pushed the issue-173 branch from 51f938e to ce357ff Compare March 12, 2018 14:36

smacker reviewed Mar 12, 2018

View reviewed changes

dpordomingo changed the title ~~[WIP] Experiments management, frontend code~~ Experiments management, frontend code Mar 12, 2018

dpordomingo changed the title ~~Experiments management, frontend code~~ Manage Experiments from the front Mar 12, 2018

dpordomingo mentioned this pull request Mar 12, 2018

Add UAST to old FilePairs #214

Closed

12 tasks

smacker reviewed Mar 13, 2018

View reviewed changes

smacker suggested changes Mar 13, 2018

View reviewed changes

smacker reviewed Mar 14, 2018

View reviewed changes

dpordomingo approved these changes Mar 14, 2018

View reviewed changes

smacker approved these changes Mar 14, 2018

View reviewed changes

carlosms force-pushed the issue-173 branch from 435f086 to 46729e6 Compare March 14, 2018 17:53

carlosms merged commit 11d7ad9 into src-d:master Mar 14, 2018

carlosms deleted the issue-173 branch March 14, 2018 17:54

dpordomingo mentioned this pull request Mar 15, 2018

Improve UX on Experiment Manager #219

Open

Manage Experiments from the front #195

Manage Experiments from the front #195

Uh oh!

Conversation

carlosms commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosms commented Mar 9, 2018

Uh oh!

dpordomingo commented Mar 9, 2018

Uh oh!

smacker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardobaeta commented Mar 12, 2018

Uh oh!

carlosms commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

smacker Mar 14, 2018 •

edited

Loading

dpordomingo left a comment •

edited

Loading