Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Shared files #121

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Shared files #121

wants to merge 12 commits into from

Conversation

georgehrke
Copy link
Contributor

replaces #120

NacreData and others added 6 commits March 10, 2016 16:14
Currently lucene only searches files I own. I add searches across shared files as well, one approach is to build one master index and then use user-specific sub-searches to limit the results. This commit is a first step down that path.
…ink to version 9.0 format (not compatible with version 8, I believe)
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @butonic, @DeepDiver1975 and @nickvergessen to be potential reviewers

}

public function logit($text) {
$FH = fopen('/var/www/html/data/borealis.log', 'a');
Copy link
Contributor

Choose a reason for hiding this comment

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

?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will just use logger instead

// FIXME: this next four lines seem a bit awkward. The goal is to get the "path" as
// getUsersSharingFile() method expects it, which is relative to the user's dir
// of files, without assuming the data directory is the same as the user name.
$full_home_dir = \OC::$server->getUserManager()->get(\OC_User::getUser())->getHome();
Copy link
Contributor

Choose a reason for hiding this comment

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

getHome called on potential null

@georgehrke georgehrke added this to the 9.0.2-current-maintenance milestone Apr 26, 2016
}

public static function postShareHook(array $param) {
if ('file' == $param['itemType']) {
Copy link
Contributor Author

@georgehrke georgehrke Apr 26, 2016

Choose a reason for hiding this comment

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

  • ===

@georgehrke
Copy link
Contributor Author

  • squash when everything is okay to go

@georgehrke
Copy link
Contributor Author

cc @NacreData

@NacreData
Copy link

I've read through all your diffs. Thanks so much for reading through and correcting all my code so carefully! I have learned several things. I am truly sorry about leaving the debugging code in and the style violations, I will make a point of being more careful in the future. Everything you've done makes sense to me and seems correct, presuming that \OC::$server->getUserSession()->getUser()->getUID() always returns the same value as that stored in the "share_with" column of the oc_share table.

What more can I do to help at this point?

@georgehrke
Copy link
Contributor Author

I think this only needs some good testing at this point.

NacreData added a commit to NacreData/search_lucene that referenced this pull request Apr 27, 2016
@NacreData
Copy link

I did a fresh install and patch with the patch-set from this pull request and found a couple of things.

patch0 bumps the apps' version number up and uses this in the internal checking to see if the index needs to be rebuilt. This seemed to be necessary to get the app to use the new index location in the upgrade I did.

patch1 catches an error which cropped up when we changed the meaning of the variable $user in one file.

patch1.txt
patch0.txt

@PVince81 PVince81 modified the milestones: 9.1-current, 9.0.2-current-maintenance Apr 29, 2016
@NacreData
Copy link

Here's small patch which simplifies some of the code I was unhappy with.
patch2.txt

@NacreData
Copy link

My most up-to-date version is at https://github.com/NacreData/search_lucene/tree/shared-files-2

In an unrelated (I think) issue, we were having problems with the cron-driven indexing. I have gotten cron indexing to work again by including one additional patch, NacreData@65615c6

The version we are using in production (which works) including all of the above is at https://github.com/NacreData/search_lucene

Bump? @PVince81 @georgehrke

@butonic
Copy link
Contributor

butonic commented Jun 16, 2016

@NacreData thx for your work. I see three problems:

  1. You decompose groups to individual users (because core has no good way of getting the raw share recipients including groups) and only store the users in the index. There is no mechanism to detect group membership changes and update the index accordingly. Better store the groups instead of individual users.
  2. When you retrieve the results you filter results after they have been returned. What when the first 10000 results are not readable for the current user. Better put the user and his groups into the query. That will however require switching to the more fancy query term object instead of just find()
  3. files and folder results are treated the same, so when you get a folder result you land in the parent folder and it will scroll to the folder. Most users just want to land inside the folder

@DeepDiver1975 DeepDiver1975 modified the milestones: 9.2, 9.1 Jul 22, 2016
@PVince81 PVince81 modified the milestones: backlog, 10.0 Apr 6, 2017
@PVince81
Copy link
Contributor

PVince81 commented Apr 6, 2017

moving to backlog for now

@butonic please evaluate how / when to move forward with this

@butonic butonic mentioned this pull request Apr 12, 2017
@butonic butonic self-assigned this Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants