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

DDF-4463 Simplify search forms selection with a dropdown #4227

Merged
merged 49 commits into from
Jan 31, 2019

Conversation

mojogitoverhere
Copy link
Contributor

@mojogitoverhere mojogitoverhere commented Jan 21, 2019

What does this PR do?

  • Simplifies the search form selection to be a simple list
  • Moves Basic and Text search out of the forms selection and into the initial query dropdown with Advanced search.
  • Refactors the code so that only one collection of search forms are maintained and only one network request is made to fetch them.
  • Starts the process of migrating the search forms to React
  • Also touches some of the result forms code, which is currently coupled to search forms

Who is reviewing it?

@willwill96 @rymach @gjvera @djblue

Select relevant component teams:

@codice/ui

Ask 2 committers to review/merge the PR and tag them here.

@andrewkfiedler
@bdeining

How should this be tested?

Basic/Text Searches
  1. Ingest some data and verify Basic and Text searches work as expected
Search Forms
  1. Click "Use another search form" and ensure "No search forms are available" is shown
  2. Copy some forms into etc/forms and run 'forms:load'. Ensure the form titles are shown in "Use another search form". Click on a form and verify the query changes as expected.
  3. Go into the "Search Forms" submenu (hamburger). Verify all previous behavior works as expected (change default forms, create a new form, delete a form, share a form, etc)
Result Forms
  1. Ensure result forms still work as expected (create a new form, share a form, delete a form, etc)

Any background context you want to provide?

What are the relevant tickets?

DDF-4463

Screenshots

Basic and Text Search
screen shot 2019-01-21 at 9 38 41 am

Search Forms
ezgif com-crop

Checklist:

  • Documentation Updated
  • Update / Add Threat Dragon models
  • Update / Add Unit Tests
  • Update / Add Integration Tests

Notes on Review Process

Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.

Review Comment Legend:

  • ✏️ (Pencil) This comment is a nitpick or style suggestion, no action required for approval. This comment should provide a suggestion either as an in line code snippet or a gist.
  • ❓ (Question Mark) This comment is to gain a clearer understanding of design or code choices, clarification is required but action may not be necessary for approval.
  • ❗ (Exclamation Mark) This comment is critical and requires clarification or action before approval.

@Schachte
Copy link
Contributor

Do we want a way to differentiate between system, shared, owned still somehow?

Copy link

@Bdthomson Bdthomson left a comment

Choose a reason for hiding this comment

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

Nice.

@andrewkfiedler
Copy link
Contributor

Do we have time to update it to work similar to how it worked here (#2906)?

I think something like this would work: https://codepen.io/andrewkfiedler/full/gqOazY
image

I think the filter capability will be important, as they're likely to have a good amount of search forms to choose from.

Copy link
Contributor

@andrewkfiedler andrewkfiedler left a comment

Choose a reason for hiding this comment

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

The comment about deleting the type attribute by no means needs to be done in this PR, that's quite a mountain of work. However, it's causing us a world of pain and needs to be dealt with sooner rather than later if we have bandwidth.

@mojogitoverhere
Copy link
Contributor Author

@andrewkfiedler I added the filter to the dropdown (thanks @djblue!) but I'm going to leave the menu layout of Text, Basic, Advanced, and Use Another Search Form as is for now (i.e. they won't be nested under Use Another Search Form)

@mojogitoverhere
Copy link
Contributor Author

build now

@bdeining
Copy link
Member

@andrewkfiedler we already have a ticket for this request #4227 (comment)

@cxbot
Copy link

cxbot commented Jan 22, 2019

Internal build has been scheduled, your results will be available at build completion.

@codice codice deleted a comment Jan 23, 2019
@cxbot
Copy link

cxbot commented Jan 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6102/
✅ JOB SUCCESS

@mojogitoverhere
Copy link
Contributor Author

build now

@cxbot
Copy link

cxbot commented Jan 23, 2019

Internal build has been scheduled, your results will be available at build completion.

@gjvera
Copy link
Contributor

gjvera commented Jan 23, 2019

🎉 Hero Successful 🎉

Quick built/installed
Basic and text search work as expected
Created several search forms with different filters
search forms all performed as expected
marking forms as default, deleting, and sharing forms all worked as expected
When there's too many search forms in the list the list will scroll
the filter works in the dropdown
Result forms worked as for sharing, deleting, and using.

LGTM

@cxbot
Copy link

cxbot commented Jan 23, 2019

Refer to this link for build results (access rights to CI server needed):
https://jenkins.codice.org/job/DDF-Jobs/job/pr/job/Linux/6107/
✅ JOB SUCCESS

Copy link
Member

@bdeining bdeining left a comment

Choose a reason for hiding this comment

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

@andrewkfiedler please re-review

@andrewkfiedler andrewkfiedler dismissed their stale review January 29, 2019 12:16

Comments are addressed but I haven't had time to rereview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet