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

New search standard and user search #9912

Merged
merged 7 commits into from
Jun 19, 2018
Merged

New search standard and user search #9912

merged 7 commits into from
Jun 19, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 19, 2018

This is a tricky one!
Fixes #2657

  • Added support for user search in the new users vue list
  • Added support for simple registration of a search in javascript
    • new OCA.Search(searchfunction, resetfunction)
    • Any app can register this with plain pure js and simply add its manager.
  • Deprecated OCA.Search is now OCA.Search.Core to ensure devs migrate their search system to this as well.
  • Fixed design of the global search in Files

@MorrisJobke
Copy link
Member

Conflicts :/

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

👍 Works nicely also with multiple instances on one page.

@MorrisJobke
Copy link
Member

I added one user "test1" and searched for "test", then I got one result, then two, then three and it stopped after 4:

bildschirmfoto 2018-06-19 um 16 15 22
bildschirmfoto 2018-06-19 um 16 15 24

Same happens with two users in the result set:

bildschirmfoto 2018-06-19 um 16 16 49

@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #9912 into master will increase coverage by 0.01%.
The diff coverage is 9.75%.

@@             Coverage Diff             @@
##             master   #9912      +/-   ##
===========================================
+ Coverage     52.08%   52.1%   +0.01%     
  Complexity    25910   25910              
===========================================
  Files          1642    1642              
  Lines         95898   95721     -177     
  Branches       1318    1289      -29     
===========================================
- Hits          49950   49873      -77     
+ Misses        45948   45848     -100
Impacted Files Coverage Δ Complexity Δ
core/templates/layout.user.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/legacy/template.php 30.32% <100%> (+0.91%) 42 <0> (ø) ⬇️
core/search/js/search.js 9.52% <5.26%> (-28.49%) 0 <0> (ø)
apps/files_trashbin/lib/Expiration.php 91.93% <0%> (+1.61%) 29% <0%> (ø) ⬇️

@skjnldsv
Copy link
Member Author

@MorrisJobke done

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 19, 2018
@MorrisJobke
Copy link
Member

I found another small issue:

  • open the apps management
  • type in the search term
  • press enter
  • before: it did nothing and just searched
  • after: it does a POST request resulting in a white page

@skjnldsv
Copy link
Member Author

@MorrisJobke the app management need to be updated to this standard as well, I'll handle it in a sec :)

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jun 19, 2018
skjnldsv and others added 7 commits June 19, 2018 23:53
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: Morris Jobke <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
}
}
// Show search
document.getElementById('searchbox').style.display = 'block';
Copy link
Member

@kyrofa kyrofa Jul 13, 2018

Choose a reason for hiding this comment

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

This line is wreaking havoc on my automated tests, throwing this on the login page:

TypeError: document.getElementById(...) is null in search.js:84:4
	initialize http://localhost/core/search/js/search.js:84:4
	Search http://localhost/core/search/js/search.js:37:3
	initialize http://localhost/core/search/js/searchprovider.js:384:11
	Search http://localhost/core/search/js/searchprovider.js:34:3
	<anonymous> http://localhost/core/search/js/searchprovider.js:432:16
	_.delay/< http://localhost/core/vendor/core.js:781:14

Firefox shows this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a deeper issue as there is no real reason why the search needs to me initiated on the login page :p

@nickvergessen
Copy link
Member

Since this patch the search icon/bar is visible on any app again, although there might not be search functionality available, eg activity, talk, settings, …

Can we do something about this?

@skjnldsv
Copy link
Member Author

@nickvergessen this shouldn't. Who is registering the search on those apps?

@nickvergessen
Copy link
Member

nickvergessen commented Jul 18, 2018

I added a break point at console.debug('New search handler registered');

initialize (search.js#63)
Search (search.js#37)
initialize (searchprovider.js#384)
Search (searchprovider.js#34)
(anonym) (searchprovider.js#432)
(anonym) (underscore.js#768)

¯\(ツ)

@skjnldsv
Copy link
Member Author

This registration shouldn't be there on every page :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: search feature: users and groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants