Remove FileUploadForm from TableDialog, add csv key options

[jjnesbitt]

Fixes #15.

Depends on multinet-app/multinetjs#7.
Depends on multinet-app/multinet-server#356

[jjnesbitt]

After looking at this and how it works with the server, I think it'd actually be better to split the FileUploadForm component back into separate components for graph and table upload, as they've diverged enough to warrant it, and the current implementation of this PR is brittle. Let me know what you think @waxlamp @JackWilb

[jjnesbitt]

Sorry for the chaos, but this PR should be good for review @JackWilb @waxlamp. It's a bigger change than would be expected, because I removed the use of FileUploadForm from the TableDialog component, and implemented the behavior directly in that component. Following the discussion me and @waxlamp had offline, it seems soon after this PR we should remove that component entirely, as the GraphDialog and TableDialog have diverged enough that using the shared component is limiting and leading to bad practices.

[jjnesbitt]

Also @waxlamp this will fail to build/pass CI until a multinetjs release

[JackWilb]

Just tested locally and I wasn't able to specify a different key or upload a table without a _key field. I'm using the server on master and the client on this branch. Is that the bug you were referencing above?

[jjnesbitt]

Just tested locally and I wasn't able to specify a different key or upload a table without a _key field. I'm using the server on master and the client on this branch. Is that the bug you were referencing above?

Do you get a specific console error? This will also probably fail if you haven't run yarn link on multinetjs

[JackWilb]

I hadn't ran yarn link, thanks for the reminder. Looks to be working on my end now. I'll review in a moment

[waxlamp]

Looking good.

A couple of UI tweaks:

• the switch should be a checkbox
• the key field input's title should be "Key Field", and it should use a placeholder/default value of _key

A bigger UI tweak for later:

• ultimately, the key field input should be a dropdown; this would be a nice UX boost since currently you have to really know what this input is for in order to use it (I will file an issue for this)

And one major logic problem:

• ArangoDB has rules for what text can go in a key field (https://www.arangodb.com/docs/stable/data-modeling-naming-conventions-document-keys.html) and these should be tested as part of validation
  • I tried using a non-complying key field and it wound up doing the upload, but refusing to show me the uploaded data when I clock on the resulting table

[waxlamp]

One more general comment. This PR seems to combine two different updates to the codebase. In general, I really prefer that orthogonal changes occur in separate PRs, even if the second PR begins at the tip of the first. If instead the two big changes are related, then I'd like for the title of the PR to express the main change being made, while the description can and should go into more detail about the secondary change and how it relates to the main change.

I realize this seems like more work. However, the effect of mashing together qualitatively different changes into a single PR is to necessarily make the changeset larger, and thus make reviewing more difficult. In my opinion, the nominal burden of organizing PRs by topic is very small compared to the excess mental burden on the reviewers when this organizing isn't done.

Let me know what you think.

[jjnesbitt]

Let me know what you think.

I agree, although I don't think it's work the effort to do that now on this PR.

[waxlamp]

Let me know what you think.

I agree, although I don't think it's work the effort to do that now on this PR.

Right, I didn't mean to make any such changes on this branch. Let's do more of this in the future though.

[jjnesbitt]

• the switch should be a checkbox
• the key field input's title should be "Key Field", and it should use a placeholder/default value of _key

I've addressed these and will push them soon.

A bigger UI tweak for later:

• ultimately, the key field input should be a dropdown; this would be a nice UX boost since currently you have to really know what this input is for in order to use it (I will file an issue for this)

That'd be cool. The question is should we do the parsing of that server-side or client-side.

And one major logic problem:

• ArangoDB has rules for what text can go in a key field (https://www.arangodb.com/docs/stable/data-modeling-naming-conventions-document-keys.html) and these should be tested as part of validation

  • I tried using a non-complying key field and it wound up doing the upload, but refusing to show me the uploaded data when I clock on the resulting table

It looks like it's referring to the keys themselves, correct?

I was able to replicate this behavior (by supplying invalid key entries), but there's nothing we could do about this client-side without analyzing the csv file correct?

[jjnesbitt]

With this recent commit I also added a restore button to that text-field, as I realized it'd be nice to just reset it if you accidentally changed it.

[waxlamp]

• the key field input's title should be "Key Field", and it should use a placeholder/default value of _key

I've addressed these and will push them soon.

The text field's title is now "Key" when it should be "Key Field" or better, "Key Column".

A bigger UI tweak for later:

• ultimately, the key field input should be a dropdown; this would be a nice UX boost since currently you have to really know what this input is for in order to use it (I will file an issue for this)

That'd be cool. The question is should we do the parsing of that server-side or client-side.

Let's take that up in the issue itself (#61).

And one major logic problem:

• ArangoDB has rules for what text can go in a key field (https://www.arangodb.com/docs/stable/data-modeling-naming-conventions-document-keys.html) and these should be tested as part of validation

  • I tried using a non-complying key field and it wound up doing the upload, but refusing to show me the uploaded data when I clock on the resulting table

It looks like it's referring to the keys themselves, correct?

I was able to replicate this behavior (by supplying invalid key entries), but there's nothing we could do about this client-side without analyzing the csv file correct?

Yes, the rules refer to the key values themselves. This is something we can check as part of upload validation. We'd need to add a phase to the validation function that checks each key value against the ArangoDB rules (which I believe we could do pretty simply by compiling a regex and checking that each key value matches it).

This is important because the current behavior is to act as though the upload succeeded, when in actuality it leaves the system in a broken state.

[jjnesbitt]

The text field's title is now "Key" when it should be "Key Field" or better, "Key Column".

Whoops, must have missed that. I just pushed that change.

Yes, the rules refer to the key values themselves. This is something we can check as part of upload validation. We'd need to add a phase to the validation function that checks each key value against the ArangoDB rules (which I believe we could do pretty simply by compiling a regex and checking that each key value matches it).

This is important because the current behavior is to act as though the upload succeeded, when in actuality it leaves the system in a broken state.

Gotcha, I'll make I've made an issue for that in the server repo.

[waxlamp]

Nice work on this.