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

EZP-25792: Fix index sub-items after main location delete #1658

Conversation

galileo
Copy link
Contributor

@galileo galileo commented May 30, 2016

Integration test that are required for a solar bundle which you can find here:

ezsystems/ezplatform-solr-search-engine#46

@galileo galileo changed the title EZP-25792: Fix index sub-items after main location delete [WIP] EZP-25792: Fix index sub-items after main location delete May 30, 2016
@bdunogier
Copy link
Member

bdunogier commented May 30, 2016

Do you expect the test to pass, or does it have requirements on pending pull-requests ?

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from e74b2f1 to 59c53c4 Compare May 31, 2016 08:16
@galileo
Copy link
Contributor Author

galileo commented May 31, 2016

Trully said I don't know why this tests are not passing, I just added some integration tests in this PR and there are not passing test with reference passing in php 7 and now I got some date issues.

Still don't know how this is connect to my changes. The fail should appear in my new integration test for solar engine but for all others should be fine.

Any idea about this @bdunogier ?

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from ac00758 to 262303f Compare May 31, 2016 14:23
@bdunogier
Copy link
Member

The failures are fixed by #1662 are they not ?

@galileo
Copy link
Contributor Author

galileo commented Jun 1, 2016

Yes they are but still I need to have those fixes in repo and then check if this solves all things.

@bdunogier
Copy link
Member

#1662 merged to 6.3 and master. You can merge the fix into this one and go ahead.

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch 2 times, most recently from ca6de07 to 2ce2e58 Compare June 1, 2016 08:33
@galileo
Copy link
Contributor Author

galileo commented Jun 2, 2016

Test should fail in this place where they are failing currently, we need the new version of solr bundle that is placed in here, what is the approach to handle such cases in our stack ?

ezsystems/ezplatform-solr-search-engine#46

ping @bdunogier @andrerom

@andrerom
Copy link
Contributor

andrerom commented Jun 2, 2016

  1. regarding question in tests and creating data, be aware there are helper methods in BaseContentServiceTest that we could consider reusing here, and/or adapt to make reusable.
  2. regarding cross repo testing, in this case we need to fix ES, as i it in this repo. As for Sorl, best practice is to temporary modify composer.json (remember to update lock file on this repo in same commit) to point to branch and alias it to version we expect. And once both sides are green and passed review, merger should merge manually w/o that commit on both sides and re start tests jobs once both are merged to verify all is green, as for this repo composer.lock needs update on merge so solr will have to be merged first.

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from 2ce2e58 to 39e5fad Compare June 6, 2016 11:29
@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from 39e5fad to 1d4432e Compare June 6, 2016 11:31
@andrerom
Copy link
Contributor

andrerom commented Jun 6, 2016

For ES, see: https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Publish/Core/Search/Elasticsearch/Content/Handler.php#L285

Seems the 2. Update (reindex).. part might need update to it's algorithm, or one of the others might by mistake delete it, for how to test and debug maybe @pspanja has some pointers.

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch 2 times, most recently from 879a597 to f922a37 Compare June 8, 2016 08:36
@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from f922a37 to 719695f Compare June 8, 2016 08:38
@galileo
Copy link
Contributor Author

galileo commented Jun 8, 2016

I fixed the ES instance but I don't have idea how to point this kernel to the solr engine that is improved here ezsystems/ezplatform-solr-search-engine#46

@andrerom you wrote:

As for Sorl, best practice is to temporary modify composer.json (remember to update lock file on this repo in same commit) to point to branch and alias it to version we expect.

But which composer.json and .lock file should I update, In the kernel there is no information about solr bundle. I can not find how this repo know something about the solr bundle. Can you explain this to me? Because now just the test for solr are not passing.

@galileo galileo force-pushed the EZP-25792-fix-index-sub-items-after-delete branch from 775259e to 02a5af6 Compare June 8, 2016 10:32
@andrerom
Copy link
Contributor

andrerom commented Jun 8, 2016

Solr; TMP commit* with the following change in bin/.travis/prepare_unittest.sh:

-    composer require --no-update ezsystems/ezplatform-solr-search-engine:dev-master
+    composer require --no-update ezsystems/ezplatform-solr-search-engine:dev-EZP-25792-fix-index-sub-items

You might have to inline alias in some cases when you update such lines for testing, but I don't think you have to in this case.

  • we'll remove it on merge

@galileo
Copy link
Contributor Author

galileo commented Jun 8, 2016

Ok this is ready to review

ping @pspanja @andrerom

@galileo galileo changed the title [WIP] EZP-25792: Fix index sub-items after main location delete EZP-25792: Fix index sub-items after main location delete Jun 8, 2016
@andrerom
Copy link
Contributor

andrerom commented Jun 8, 2016

+1, congrats @galileo ;)

@andrerom andrerom closed this in 8df550b Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants