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

Filter blocks UI (1) #247

Merged
merged 34 commits into from
Dec 1, 2020
Merged

Filter blocks UI (1) #247

merged 34 commits into from
Dec 1, 2020

Conversation

alexkb0009
Copy link
Contributor

@alexkb0009 alexkb0009 commented Oct 22, 2020

(Outdated Pic, more features/etc now:)

Current Implementation Notes

  • Decided to re-implement the 'ActiveFiltersBar' since might be able to create better / more-responsive layouting using flexboxes now. (And is not really much code, esp JS-side). (Not fully complete).

Next Challenges

Will need to work on FilterSetController to:

  • store currently selected FilterSet Block - done mostly
  • if multiple selected then don't show FacetList
    • (De)Selection will occur on click (?)
    • Update: for now for simplicity, allowing only 1 (or 0 (aka 'all')) blocks to be selected.
  • if 1 selected, then show FacetList AND display current search query/filters rather than saved one (since we'll save this, if we're editing)
  • Will likely hold a 'not yet saved' FilterSet Item in memory and apply changes to it as they're made in FacetList... until saved..
    • Done & done
  • Need to figure out intricacies of above

All Completed.

Remaining Fixes

Need to (a) fix CustomColumnSelector/Controller re: compound filter sets (and EmbeddedSearchViews as a whole) and Done in SPC master, will update SPC version to 0.0.2.83 before merging. (b) figure out if we save 'sort' param to filterset or similar (probably not.. for time being at least) sort is currently not saved anywhere on FilterSet, we completely lose 'sort' if change to a different filterblock/compound-request.

Possible ToDos

  • AJAX in active_filterset by its @id to avoid needing to embed extra fields on Case... completed.
    • This might be simpler to do after rest of stuff is complete, including searchbox for selecting previous/other filtersets; could perhaps optimize to re-use search result if we request limit=all or sort=date_created&sort=actively_used something...

Copy link
Contributor

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

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

Some preliminary comments/questions that I hope are helpful as you continue to work on this.

Comment on lines 45 to 51
# sub_request.environ = env # copy back in
sub_request.__parent__ = None # XXX: set parent request (is None *always* correct?) -Will
sub_request.registry = parent_request.registry # transfer registry as well
if hasattr(parent_request, "context"):
sub_request.context = parent_request.context
else:
sub_request.context = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go ahead and persist this change - you can get rid of the commented out code.

Comment on lines 65 to 76

#env = parent_request.environ.copy() # copy parent request
#env['PATH_INFO'] = sub_request.environ['PATH_INFO'] # override what it does
#env['QUERY_STRING'] = sub_request.environ['QUERY_STRING']

if '?' not in query: # do some sanitization
query = '?' + query
subreq = Request.blank(route + '%s&from=%s&limit=%s' % (query, from_, to))
# subreq = Request.blank(route + '%s&from=%s&limit=%s' % (query, from_, to))

subreq = make_subrequest(request, route + '%s&from=%s&limit=%s' % (query, from_, to), "GET")
subreq.headers['Accept'] = 'application/json'

Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here - remove commented out code, keep your change


def build_initial_columns(used_type_schemas):

columns = OrderedDict()
Copy link
Contributor

Choose a reason for hiding this comment

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

All Python 3.6 dictionaries are OrderedDicts, so can just use columns = {} but can keep the specificity if you want.

@@ -371,3 +372,43 @@ def is_numerical_field(field_schema):
def is_date_field(field, field_schema):
""" Helper method that determines if field_schema is """
return determine_if_is_date_field(field, field_schema)

def build_initial_columns(used_type_schemas):
Copy link
Contributor

Choose a reason for hiding this comment

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

This I think should be a @staticmethod on SearchBuilder, unless you think this has external use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure but probably not... yeah staticmethod makes sense for it.. will change

Comment on lines 394 to 396
# If @type or display_title etc. column defined in schema, then override defaults.
for prop in schema_columns[name]:
columns[name][prop] = schema_columns[name][prop]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think columns[name].update(schema_columns[name]) is what you want here?

// // TODO: Show single > or < or something.
// }

const fieldSchema = getSchemaProperty(field, Schemas.get(), "VariantSample");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we bake assumptions in like this? It would be ideal if filter_sets were generic client side as well that way they can be used for other things (in the future potentially).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In long run yeah I think that might be good idea but for time being was working under impression FilterSetUI would only apply to table of variant samples so left some things hardcoded re: that since results in less complex code. Once/if we do need to repurpose for other search_types, it shouldn't be much issue to parameterize-upon / allow-dynamic-value-for it though.

"@id" : PropTypes.string, // Is required if originally existed, else free to be null.
"uuid" : PropTypes.string, // Is required if originally existed, else free to be null.
"title" : PropTypes.string.isRequired,
"search_type" : PropTypes.oneOf(["VariantSample", "Variant", "Case"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PropTypes don't throw any hard errors, but do generate warnings in browser console in dev environments. Generally we use them as if were documentation/reminders for type definitions in place of like JSDoc.

Comment on lines +817 to +822
const excludedFieldMap = {};
if (Array.isArray(excludeFacets)) {
excludeFacets.forEach(function(field){
excludedFieldMap[field] = true;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just make this a dictionary from the onset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.. legacy compatibility mostly... this might be a good opportunity to change it, though also hasn't been much of an issue. Will try to maybe do depending on time/other-stuff.

Comment on lines +884 to +888
this.addNewFilterBlock = _.throttle(this.addNewFilterBlock.bind(this), 750, { trailing: false });
// Throttled, but func itself throttles/prevents-update if still loading last-selected search results.
this.selectFilterBlockIdx = _.throttle(this.selectFilterBlockIdx.bind(this), 250, { trailing: false });
// Throttled to prevent accidental double-clicks.
this.removeFilterBlockAtIdx = _.throttle(this.removeFilterBlockAtIdx.bind(this), 250, { trailing: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as previously WRT throttle values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mix of 'low enough to not impact usability during functional/real usage' and 'high enough to prevent people rapidly clicking it a lot and driving up server load with lot of search requests'. (Almost all of these methods ultimately trigger search query).


this.state = {
"currFilterSet" : { ...initialFilterSetItem },
// todo: maybe change to allow multiple in future?
Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely think this is a feature that will be requested ie: quickly swap from filter_set_1 to filter_set_2 etc.

@alexkb0009 alexkb0009 mentioned this pull request Nov 18, 2020
Copy link
Member

@Bianca-Morris Bianca-Morris left a comment

Choose a reason for hiding this comment

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

Still haven't tested this on my local machine, so don't want to nitpick too much until I understand more of how this works/looks. But will soon (test, not necessarily nitpick, ha). Is there a related branch on SPC I need to test it with?

const duplicateNameIndices = {};

filter_blocks.forEach(function({ name, query }, idx){
var i;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this can be confusing. var can be used to make a variable global, if it's defined in global scope. But here, it's defined inside of a function, so i is local to the function, but isn't available outside of it. let is typically scoped to the block (for loop, conditional statement, etc.), but since it would be defined outside of the loops here, that wouldn't make much of a difference. So, var is fine in this case, but let would work, too.

Typically better to use let to avoid bugs unless there's a scope-related reason to use var. So you're actually kinda right, but it's for a different reason.

Comment on lines +182 to +190
if ( // Rebuild tooltips after stuff that affects tooltips changes.
pastFilterSet !== currFilterSet || // If FilterSet changes, then some tips likely for it do as well, esp re: validation/duplicate-queries.
selectedFilterBlockIdx !== pastSelectedIdx ||
(bodyOpen && !pastBodyOpen) || // `data-tip` elems not visible until body mounted in DOM
cachedCounts !== pastCachedCounts // Tooltip on filterblocks' counts indicator, if present.
) {
setTimeout(ReactTooltip.rebuild, 0);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for reminding me how to do this. I keep running into tooltip-related woes, and forgot you could do a rebuild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A small consideration in back of my head has been what's performance of these ReactTooltip.rebuild calls. By its nature, I think it traverses the entire DOM tree to find these 'data-tip' attributes, which does not seem it might be necessarily fast.

Just profiled it out of curiosity and seems it only takes about 3ms though, during otherwise idle time (thanks to the setTimeout), not super fast but good. One random idea if it were an issue might be to debounce the ReactTooltip.rebuild func so if called multiple times within some period of time, gets called only once at tail end of that period.

Comment on lines +69 to +72
EmbeddedItemSearchTable.defaultProps = {
"columnExtensionMap": columnExtensionMapCGAP,
"facets" : undefined // Default to those from search response.
};
Copy link
Member

Choose a reason for hiding this comment

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

Why explicitly set facets prop to undefined? Won't props typically default to that if nothing is passed through, anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for explicitness / documentation-for-self mostly. I figured if is explicitly set to undefined then "undefined" feels more of a valid value, rather than having it feel like some accidental/overlooked lack of value which then feels like some sort of issue to be solved (especially if come back to this code after long time of not working on it) rather than an explicit design choice that allows/handles "undefined"... idk. Lmk if other thoughts, am def up to changing this.. and/or documenting.. etc.

Comment on lines +1214 to +1219
if (!React.isValidElement(child)) { // String or something
return child;
}
if (typeof child.type === "string") { // Normal element (a, div, etc)
return child;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not merge these two into one if ( x || y ) statement? If you wanted to keep the comments, you could line break after or and add after each expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah no real reason to not merge em.. will do; I think was just in some sort of habit of writing blocks of code that test + return out if some invalid data, condition-by-condition.

Comment on lines 358 to 359
if filter_set.get(CompoundSearchBuilder.TYPE) not in request.registry[TYPES]["FilterSet"].schema["properties"][CompoundSearchBuilder.TYPE]["enum"]:
raise HTTPBadRequest("Passed bad {} body param: {}".format(CompoundSearchBuilder.TYPE, filter_set.get(CompoundSearchBuilder.TYPE)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to cause tests to fail if you don't add the types used in testing to the schema.

Comment on lines 145 to 148
# The below didn't work, I guess no request params.. which makes sense..
#result.search_base = result.normalize_query(request, result.types, result.doc_types)
#result.prepared_terms = result.prepare_search_term(request)
#result.set_sort_order()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed now?

@alexkb0009 alexkb0009 marked this pull request as ready for review November 20, 2020 16:53
@alexkb0009
Copy link
Contributor Author

Is deployed now to cgaptest & waiting for indexing to complete.

Copy link
Contributor

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

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

Approving with small comments - one thing to note is that this may interact weirdly with the new range facets, so may be worth loading up one of those and testing it out. Also needs a version bump in pyproject.toml.

Comment on lines +290 to +292
let searchHrefBase = "/search/?type=VariantSample";
searchHrefBase += initial_search_href_filter_addon ? "&" + initial_search_href_filter_addon : "";
searchHrefBase += "&sort=-date_created";
Copy link
Contributor

Choose a reason for hiding this comment

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

My instinct tells me to watch out for code like this (repeated string building). Strings are still immutable in JS, yes? So why not just do the below? This reduces it from 3 to 2 builds I think. You can reduce it to 1 by including the & in the initial_search_href_filter_addon, which I think would be smart (assuming this isn't optimized later on somehow).

let searchHrefBase = "/search/?type=VariantSample&sort=-date_created";
searchHrefBase += initial_search_href_filter_addon ? "&" + initial_search_href_filter_addon : "";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I might condense this a little.. we do join strings a lot in general in JS tho (i.e. HTML className strings changing re: some state) and this particular case is not really detrimental to performance (it happens once during init for most part, not like 10000 loops of this).

I'd prefer to not bundle in '?' or trailing '&' etc into stored URL chunks since IMO then becomes harder to keep track of what variables are expected to have what prefixes or suffixes; worried that would probably (from personal experience) resort to checking like if not string then ... else if string.endswith("?") or endswith("&") then .. else ... -type stuff which gets even messier lol. Easier/as-easy/cleaner to just do stuff like pathName + (urlQueryChunks ? "?" + urlQueryChunks.join("&") : "") in place(s) where full href is formed/to-be-used.

Comment on lines +352 to +353
// Load initial filter set Item via AJAX to ensure we get all @@embedded/calculated fields
// regardless of how much Case embeds.
Copy link
Contributor

Choose a reason for hiding this comment

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

This means we can/should be using only default embeds on case then, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least for this active_filterset field, yes... for other things like Pedigree Families I assume the embeds are still needed. We could in future though AJAX in more of the Case rather than embedding it (especially the stuff/data that is click(+) away). Like AJAX in Family (with all its members) and then render the Pedigree visualization.

* linkTos must be transformed to UUIDs before POST as well.
* Hardcoded here since UI is pretty specific to it.
*/
const fieldsToKeepPrePatch = ["title", "filter_blocks", "search_type", "flags", "created_in_case_accession", "uuid", "status"];
Copy link
Contributor

Choose a reason for hiding this comment

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

global_flags is the way to quickly pass a sub-query to all filter blocks. For example you might want to apply AF < .001 to all filter blocks - your option is to either go through each filter block and add it individually or set global_flags. Not adding it to the schema is probably an oversight on my part, as global_flags was added in the second iteration. Odds are you'll want to use this functionality (and save it).

Comment on lines +429 to +432
<form className="d-flex align-items-center mb-0" action="#case-info.filtering" onSubmit={function(e){
e.stopPropagation();
e.preventDefault();
setIsEditingTitle(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it convention to use anonymous functions for things like this (and not organize them somehow as named functions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope... especially not when are passing functions down to named Components since this will cause memoized/PureComponents to re-render each time (since brand new func passed down each time). It becomes less of a concern when is passed into a JSX element (lowercase JSX) (as opposed to React Component (capitalized JSX)) as is here (into "<form ..") since there's little/less memoization optimization taking place there (JSX elements are terminal "leaves" of the React component tree), and especially if the parent Component itself is memoized and prevents unnecessary re-renders.

This anonymous function is the way it is (not named class method or a named memo-generated function) mostly due to the above (the performance benefit is not too significant if formalize it into named func) and due to speed of implementing it. As am polishing it now though, yeah making it a named function would be good/better practice/convention.

"active_filterset.filter_blocks.query",
"active_filterset.filter_blocks.flags_applied",
"active_filterset.flags",
"active_filterset.@id",
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary, if it is a linkTo in schema then @id will be included as a default embed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I might've been figuring to be explicit in its requirement but I guess we have schema for that also.

return merge_query_strings(qstring1, qstring2)
for k, v in dict_with_more_vals.items():
if k in dict_to_merge_into:
dict_to_merge_into[k] += v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appears that would get auto-unique'd by urllib.parse.urlencode -- not tested, though.

@@ -142,8 +142,6 @@ def from_search(cls, context, request, search, from_=0, size=10):
:return:
"""
result = cls(context, request, skip_bootstrap=True) # bypass (most of) bootstrap
result.from_ = from_
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe should have kept this and then done result.search = result.search[from_:from_ + to]here also ... idk.

Copy link
Member

@Bianca-Morris Bianca-Morris left a comment

Choose a reason for hiding this comment

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

Works well. I've already shared my thoughts on look/feel on Slack; looking forward to getting some more feedback on that. I know they'll be excited to test!

Although second thought, maybe double check those Travis logs. Doesn't look like there's a lot of info in there as to why it's failing, so maybe just Travis being flaky? Second pair of eyes can't hurt.

# Returns a generator as value of es_result['hits']['hits']
# Will be returned directly if self.return_generator is true
# or converted to list if meant to be HTTP response.
# Theoretical but unnecessary future: Consider allowing to return HTTP Stream of results w. return_generator=true (?)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ Basically describes functionality of metadata_tsv, don't think any good use case for streaming JSON objects or similar tho.

@alexkb0009 alexkb0009 merged commit b32ce5e into master Dec 1, 2020
@alexkb0009 alexkb0009 deleted the filter-blocks-ui-1 branch December 1, 2020 21:48
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.

None yet

3 participants