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

TASK-1128 Introduces a dropdown that populates itself from the staging area #1191

Merged
merged 19 commits into from
Oct 26, 2017

Conversation

briehl
Copy link
Member

@briehl briehl commented Oct 20, 2017

MERGE #1177 FIRST!

(this includes tests built on the newer configuration)

This is getting a little large, so starting a PR here.
The goal is to create an import widget for an App Cell. This should be a dropdown menu that populates itself by searching for files in the FTP staging area.

The next PR will introduce a mechanism for it to query against dynamic services. But that will likely require some updates to the Narrative Method Store to get the parameters in the App Spec, so it's not yet.

This creates a new parameter type in App Specs called "dynamic_dropdown". This gets interpreted as a string and entered into the Python app running code as a file path. It makes use of Select2, so that when a user clicks the dropdown, it'll do a search against the FTP service (assumes a working FTP endpoint with a '/search/' route) and populate the dropdown based on that search. Also assumes the results are of a specific structure, but that'll also change when this gets adapted for dynamic services.

Still todo on this PR:

  • tests
  • dynamicDropdownView.js (need a view-only mode)
  • revert to FTP endpoint URL in the config (oops)
  • more styling to the dropdown (I don't think "last modified" is appropriate for FTP files that are effectively untouchable after uploading. should be "uploaded" with the actual timestamp)

@briehl briehl requested a review from eapearson October 20, 2017 23:45
@briehl
Copy link
Member Author

briehl commented Oct 21, 2017

Needs tests now.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 7.855% when pulling cd557fc on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 7.855% when pulling cd557fc on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 7.855% when pulling 001f1c6 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 12.203% when pulling 7bab74c on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.2%) to 8.73% when pulling bf8d5e7 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 12.217% when pulling bf8d5e7 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@@ -105,62 +105,59 @@ define([
*/
function updateConfig() {
return new Promise.try(function (resolve, reject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be just
new Promise(function (resolve, reject) {...
also if anything throws in here (assertConfig()?) it will not cause the promise to reject, but rather the constructor will fail and throw.

});
} else {
if (window.kbconfig) {
return window.kbconfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

also, this won't resolve with the config -- it would have if this was original simply Promise.try()

console.log('Config: processing local data configuration.');
config.publicCategories = dataCategories[config.environment].publicData;
config.exampleData = dataCategories[config.environment].exampleData;
return Promise.try(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

and these Promse.trys are not necessary since they simply return a value -- just returning the value will continue the promise chain.

return Promise.try(function() {
var selectedItem = getControlValue(),
validationConstraints = {
min_length: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the validation constraints be driven from the spec or sdk.js? trawling back through memory of how this is used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

also in theory wouldn't the validation go against the data source? I mean, in actual usage the value is derived from a dropdown derived from the data source, but as far as the control knows, at some future time this may not be true. That is a problem in general for this control, though, since all files will eventually disappear and thus every instance of this parameter will become invalid. Hmm..

Copy link
Member Author

@briehl briehl Oct 26, 2017

Choose a reason for hiding this comment

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

Yes, but these aren't expected to have string constraints as it's a new parameter type, and I'm coding around some still needed changes to the narrative method store. Those are just there as default values, though sdk.js should cough up the defaults itself, instead of the input widget. I'll change that.

@@ -0,0 +1,70 @@
/*global define*/
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, parameter control specs!!

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 perfect, but it's a start! :)
I'm trying to not introduce any new code without a few tests. Hopefully, it'll become a trend...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 12.222% when pulling 2f53f95 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@briehl
Copy link
Member Author

briehl commented Oct 26, 2017

Ok, adjusted stuff with your comments, and fixed tests so they get actually get skipped when no auth is present.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 12.222% when pulling c67da11 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 12.217% when pulling c67da11 on briehl:task-1128-import-dropdown into 5f4983b on kbase:develop.

@briehl briehl merged commit 464f78c into kbase:develop Oct 26, 2017
@briehl briehl deleted the task-1128-import-dropdown branch May 8, 2018 21:37
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