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

Capital search #1705

Merged
merged 7 commits into from Feb 20, 2018

Conversation

Projects
None yet
4 participants
@benheng
Contributor

benheng commented Feb 6, 2018

Request and task filtering with all caps
For example, filtering with "HWB" would return something like this:

HelloWorldBen
LookHeyWheresBillyGoneTo

@benheng benheng added the hs_staging label Feb 6, 2018

@benheng benheng requested review from ssalinas and kwm4385 and removed request for ssalinas Feb 9, 2018

});
}
export const getStarredRequests = createSelector(

This comment has been minimized.

@ssalinas

ssalinas Feb 12, 2018

Member

were the rest of these methods unused I take it?

This comment has been minimized.

@benheng

benheng Feb 12, 2018

Contributor

Yeah none of these were used anywhere and I was wondering if it was duplicated by selectors/requests/filterSelector but now I'm not sure. It looks like it has selectors for some incomplete features, like getStarredRequests. I preferring removing unused code, but I can see how this could be helpful for completing some feature requests.

@ssalinas

Will let @kwm4385 have the final word, but seems fine to me, and will match closer to the matching in our deployer 👍

@kwm4385

This comment has been minimized.

Contributor

kwm4385 commented Feb 12, 2018

This doesn't seem to be working on staging, did you deploy after merging there?

// search heuristics to just the upper case characters of each option
const options = {
extract: Utils.isAllUpperCase(filter.searchFilter)
? (requestParent) => Utils.getUpperCaseCharacters(requestParent.id)

This comment has been minimized.

@andyhuang91

andyhuang91 Feb 12, 2018

Doesn't the exact method have to be called in the all uppercase case too?

This comment has been minimized.

@benheng

benheng Feb 12, 2018

Contributor

This is the extract method. In the uppercase case, it's returning only the capitals of each request.

@@ -74,9 +74,16 @@ export default createSelector([getRequests, getFilter, getUtilizations], (reques
});
filteredRequests = _.union(res1, res2).reverse();
} else {
_.each(filteredRequests, (requestParent) => {requestParent.id = id.extract(requestParent);});
_.each(filteredRequests, (requestParent) => (requestParent.id = id.extract(requestParent)));

This comment has been minimized.

@andyhuang91

andyhuang91 Feb 12, 2018

Returning a value in _.each shouldn't do anything so I'm not sure why this change was necessary. The existing code is pretty unclear about what this loop actually does. There appears to be some sort of side effect with the extract function. Would it be easy to clean up the existing code?

This comment has been minimized.

@benheng

benheng Feb 12, 2018

Contributor

Hmm I did seem to make it less clear since now it's returning something and we don't use it. it was unclear to begin so I'll see if I can clean it up.

This comment has been minimized.

@benheng

benheng Feb 12, 2018

Contributor

selectors/tasks has similar code and it looks like it makes more sense in that file and less sense here.

@benheng

This comment has been minimized.

Contributor

benheng commented Feb 12, 2018

@kwm4385 I pushed it to hs_staging, but forgot to deploy it. I'll do it again once I've addressed the other comments.

@kwm4385

This comment has been minimized.

Contributor

kwm4385 commented Feb 12, 2018

🚢

@andyhuang91

Just some minor comments. Overall this looks good. 👍

filteredRequests = _.union(users, ids);
} else {
// Allow searching by the first letter of each word by applying same
// search heuristics to just the upper case characters of each option

This comment has been minimized.

@andyhuang91

andyhuang91 Feb 12, 2018

Comment is in wrong place

if (Utils.isGlobFilter(filter.searchFilter)) {
const res1 = _.filter(filteredRequests, (requestParent) => {
return micromatch.any(user.extract(requestParent), `${filter.searchFilter}*`);
const users = _.filter(filteredRequests, (requestParent) => (

This comment has been minimized.

@andyhuang91

andyhuang91 Feb 12, 2018

maybe userMatches and idMatches for clarity instead of just users

tasks = _.union(hosts, ids, racks);
} else {
// Allow searching by the first letter of each word by applying same
// search heuristics to just the upper case characters of each option

This comment has been minimized.

@andyhuang91

andyhuang91 Feb 12, 2018

comment here is also in the wrong place

@benheng

This comment has been minimized.

Contributor

benheng commented Feb 12, 2018

Okay I've actually correctly deployed it to hs_staging this time.

@ssalinas ssalinas added this to the 0.20.0 milestone Feb 15, 2018

@benheng benheng merged commit 7813c11 into master Feb 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@benheng benheng deleted the capital-search branch Feb 20, 2018

@ssalinas ssalinas modified the milestones: 0.20.0, 0.19.0 Feb 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment