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

[WIP] Search Engine (issue 535) #97

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

Conversation

osarrat
Copy link
Member

@osarrat osarrat commented May 22, 2017

No description provided.

@osarrat osarrat requested a review from niksj May 22, 2017 12:34
@@ -551,6 +551,15 @@
<artifactId>mail</artifactId>
<version>1.4.5</version>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please ignore any local changes I have made. The only one which matters is this solr dependency.

pom.xml Outdated
@@ -214,7 +214,7 @@
<hibernate.dialect>org.hibernate.dialect.PostgreSQLDialect</hibernate.dialect>
<hibernate.connection.driver_class>org.postgresql.Driver</hibernate.connection.driver_class>
<hibernate.hbm2ddl.auto>update</hibernate.hbm2ddl.auto>
<hibernate.show_sql>true</hibernate.show_sql>
<hibernate.show_sql>false</hibernate.show_sql>
Copy link
Member Author

Choose a reason for hiding this comment

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

My two cents about this: instead of modifying the common pom.xml file, you should rather maintain your own Maven settings.xml file as very recently documented here: http://wiki.sigmah.org/doku.php?id=contributorguide:preparebuildenvironment#step_3your_first_build

Enjoy !

@osarrat
Copy link
Member Author

osarrat commented Jun 16, 2017

Good frequency of small commits, keep with this habit ! I makes following and understanding easier !

Two general comments:

Could you adjust your future work to improve those two issues ?

Good work so far ! Continue like that !

@halfcurry
Copy link
Contributor

halfcurry commented Jun 16, 2017

Apologies for forgetting to put the issue number each time! The codacy report mostly suggests that I clean up my unused imports. At present I am keeping them just in case I might need to import some stuff again, and I'll clean them up as soon as I'm sure I don't require them at all. And I'll soon modify my pom.xml file as well and remove local settings as you mentioned. Thank you!

@halfcurry
Copy link
Contributor

In my latest commit ( 8dd81b4 ), most projects are opening out on clicking the corresponding search result, but some are facing permission issues. Need to investigate what are the permissions preventing those specific projects from opening out.

Copy link
Contributor

@halfcurry halfcurry left a comment

Choose a reason for hiding this comment

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

"Search Filtering" in the commit 049a3ee means the filtering of the query according to need i.e Projects, OrgUnits, Contacts. Note: First-time connection error is not yet resolved. It re-occurs and the temporary fix did not work.
"Filtering" in 9d15b0a means the filtering of the results based on the permissions the currently logged in user has.

@halfcurry
Copy link
Contributor

In the commit dd095d6, I have added an auto index job which does a solr full data import. This job is initialized when the dashboard first loads ( this is a temporary solution just to test whether the job works or not ), and keeps firing every 10 minutes.

@halfcurry
Copy link
Contributor

0604656 : Should be global 'solr' core url.

@halfcurry
Copy link
Contributor

bec9e80: I have added a column in the table organization, hence added a migration sql file as mentioned here: http://wiki.sigmah.org/doku.php?id=contributorguide:contributionrules. I hope there is nothing more to change.

@halfcurry
Copy link
Contributor

It seems there is a jdk version error, i.e I am compiling with JDK 1.8 and the builder is running tests with JDK 1.6 or 1.7 : https://stackoverflow.com/questions/23249331/java-unsupported-major-minor-version-52-0
That is the reason it is not able to pass the tests.

@halfcurry
Copy link
Contributor

By "rudimentary" files results filtering, I mean that only those files which the currently logged in user is an author of are being shown in the search results. On clicking these results, the file successfully downloads. This is the best permission I am able to implement for now ( and is partially useful at the same time ), due to the complexity of the code. For future, a better permission has to be implemented.

@halfcurry
Copy link
Contributor

Hi @osarrat @niksj , I need to write a brief description / gist here as part of the final evaluation. Can I get rights for the same? Currently I am unable to write a description.

@ghost
Copy link

ghost commented Nov 21, 2017

Well, for this one i'm 50/50 about integration.

At the first sight, it seems that it's a good basis with not so much impacts, which is nice but on the other hand, while the development has been focused on building a viable POC (which is fine), non-working/limit cases seems not sufficiently considered (e.printStackTrace() instead of exception management). Also, I know that a big issue about search is about keeping the index and the database in sync (involving transaction management to only commit changes in the index if commit in the database has passed) and I know solr is very fast but sometimes corrupts its index, so being able to reset/clear the index and rebuild it (reindexing the whole database) should be available as a feature (and I didn't see it, but I only had a quick review).

Seems a good start with things in the right place through. I think a developer who would have to implement this feature should seriously consider continuing this PR, it may be faster than starting from nothing.

@osarrat
Copy link
Member Author

osarrat commented Nov 23, 2017

Thanks @numero-six for this review !

So, if I understand you correctly, your reluctance mainly come from functional issues like a lack of reset button, or improvable error messages. But you consider this piece of work as valuable enough to start to have a search engine in Sigmah.
The good thing is that this search engine feature has been developed so that it can be easily enabled or disabled to some user profiles by a single global privilege. So, in my opinion, if we consider the risk to lose this big amount of work because too different from the master, I would recommend to merge this PR and informing users that it is still an early feature.
Does it seem a good trade-off in your opinion @numero-six ?

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.

2 participants