User tests: Successful: Unsuccessful:
Pull Request for Issue # .
When the category state of a content change it was not propagated to the index of the smart search.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Category | ⇒ | Search |
The com_finder is used to create a index of words in order to use it in smart search way. We start writing some letters in the search filed and the smart search show some possible words that the user can chose. The words are from the previous index created.
To index is created based in the data associated to the com_finder. For example: articles, contacts, etc, can be associated to the smart search. For each type of item we have to activate the corresponding plugin.
When there is some change in the items to index there is some events that fire and that are responsible to actualize the index. One of that cases is when we change the state of a category of one of those elements. The problem founded here was that when we change the state of category of a item that is in the index, the function that handle this change was not working correctly. This function should reindex all the items related to the category but by mistake it was taken the wrong identifier of the items. Instead of taken the item identifier to reindex, it was taken the category identifier.
To test this you should create, for example, an article with the state published, with a specific word in is title (for example 'word_to_index') and associate it to a category that also as the state state published. Then try to find the article using the word 'word_to_index' in the main search field. It should show the article in the search results. Now try to unpublished the article and make the same search. You will see that the article still appear but it shouldn't. After make this correction and repeating all the steps described before, the article with the unpublished state will not appear this time.
Note: You should change the article category state in the edit form in the administration site. You have to also change the category access beside with the state change in order to fire the reindex of the items. This is another problem with this component that I will try to correct later.
ok thanks for the explanation and test instructions, will test when i have time
I have tested this item unsuccessfully on cd5ee81
Steps done:
The result of your test are very strange because you are testing the change state of the article and I only change the change state of the category of the article. To test my pull request you should change the state of the category "Category com_finder" from published to unpublished and not the state of the article. Nevertheless, I will simulate your test, but it must pass because was not the part of the code that I change.
sorry you are right, i was with cache enabled ....
The test should be:
Used 3.5.0 RC for the tests.
i think i see what you mean now, you have to change the category published status to unpublished on the edit page to detect the problem (not the button or checkbox in the list view, that already worked fine), but with or without your patch the behaviour is the same, in other words, doesn't remove it from the search results list.
Whilst this PR certainly looks correct, fixing it might serve to highlight a performance issue. If you publish/unpublish a category containing a lot of articles (where "a lot" depends on the server specifications) or many particularly large articles (with a similarly vague definition of "many" and "particularly large") you could be waiting a long time for a response. In some cases the PHP process may time-out before it completes.
I'm not saying we shouldn't fix it though. :-)
Try to change the access filed at the same time as you change the state to unpublished. I think in this way may patch will work.
As I said before: "You should change the article category state in the edit form in the administration site. You have to also change the category access beside with the state change in order to fire the reindex of the items. This is another problem with this component that I will try to correct later."
In resume, my patch only resolve part of the problem.
So the tests should be:
You right @chrisdavenport. But I think who use this plugin must have the information that the system could lose in performance. But that doesn't mean that we should not resolve the errors.
I have tested this item successfully on cd5ee81
It solves the part of the reindex when changing a category state AND access level (at the same time) in edit view according to comments above.
IMHO, a PR like this should solve the whole issue, in other words, also solve the fact that is not reindexing when we only change the state of the category (in edit view).
Not sure if you will find it useful or not, but some years ago I tried to put together a test plan for Smart Search which would establish the expected behaviour. Your PR reminded me of it.
https://docs.joomla.org/Smart_Search_content_change_test_plan
I create this PR because is was an error with an easy correction. I known that there is more work to do to correct all the management of category state change in the smart search. The problem is having time and knowledge to do the work, but I can try.
In your opinion @andrepereiradasilva, if I resolve the others issues you think is better to add new code to this PR or create another one?
Thanks @chrisdavenport for the information. But this test plan was accepted by the majority of the joomla community or it is some plan that where not been validated? Is a good idea use this plan to guide me to the process of the smart search correction?
IMHO for the edit published state case problem to this PR. Other call have their PR.
@miguelbgouveia I wouldn't say it has any "official" status, whatever that means, but if you were to run through it and find a discrepancy I think it would be worth opening an issue in the tracker so it can be discussed.
But as @andrepereiradasilva said, let's not cloud this issue. This PR just needs one more test.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
I think I finished the PR @andrepereiradasilva. The test should be:
This test should be repeated for contacts and newsfeeds. Just substitute articles with contacts and newsfeeds in the test text.
@chrisdavenport I found a small issue with your test plan for Smart Search.
In the content item state changes and category state change it should mention that we should test the change when it is made in the list view and also when it is made in the form. This is necessary because there are two different functions that deal with this two ways of changing the state. I try to propose this change in the test plan it self but I have not permissions for that.
Title |
|
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This last commit is to correct the issue that when a content was in the state of archive it was not showing in the results of the smart search. The archived content should be showed as said in the tests plans of @chrisdavenport. The problem of the access level is a more complex one that I also want to correct. I don't know if I, or another person, will have time to correct it in a near future. So, I suggest that we move it to another pull request. This way at least we will have the category state changes correct. What do you think @andrepereiradasilva?
ok by me, but i'm just a volunteer.
What more actions can I do to this correction be accept in some future version of Joomla?
Two people have to test successfully, them it's up to the maintainers decide when to merge it.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This PR has received new commits.
This lasted changes is to correct a new founded problem. When we have a content belonging to a category that have more than one hierarchy level, when the state of the categories of the upper levels are changed this change have to be propagated to the contents belonging to the lower levels categories.
To test this we can create an hierarchy of categories with tree levels. For example: category 1, category 1.1 and category 1.1.1, all with the state equal to publish. Then create a content with the state equal to publish and in the category 1.1.1. The content must be created with a specific characters in the title, for example xyz. If we insert the xyz characters using the smart search the content should appear in the results. Changing the category 1 state to unpublished and searching again by the xyz characters the content now should not appear in the search results. We should test this scenario using the two category state editing mods. Changing the category sate in the list of categories and using the category edit form.
I have tested this item
Tested succesfully with articles and contacts and can confirm that the article/contact is no longer presented in the search results after the patch.
I have tested this item
Tested @icampus
followed the test steps with article and contact categories. Pre-Patch search finds the article/contact. After apply patch and changing the state to unpublished the article/contact still appears in the search results. Only after a reindexing it disappears from the search results.
I have tested this item
I have tested this issue @icampus PBF with the proposed change and I get an error. I followed the suggested steps but I get an error "Warning: Header may not contain more than a single header, new line detected in C:\xampp\htdocs\bugtesting\libraries\joomla\application\web.php on line 961" when changing the category to unpublished after applying the patch.
@miguelbgouveia Can you update your PR please so the conflicts are fixed and we can test again? Thanks
Category | Search | ⇒ | Administration Components Plugins Front End Search |
Now I think is ready to test. The problem was added in the last commit that I was made.
you need to fix the conflicts
@andrepereiradasilva, how can I see where are the conflicts?
You can do it in a git cli interface, Github for Windows. Search the web.
Or you can manually check the recent history of the files you changed to see what changed and replicate those changes.
The bottom line is you need to fix conflicts and update your miguelbgouveia:patch-3
branch to latest staging.
Do you know how to use git e.g. a local client via command line?
git checkout
git pull git@github.com:/joomla/joomla-cms.git staging
[now git tryes to sync and show the conflicted files]
[now you need to fix the conflicts]
Git commit -am 'fix conflicts'
Git push origin
@zero-24 I try git pull git@github.com:/joomla/joomla-cms.git staging but I get this error:
Permission denied (publickey).
fatal: could not read from remote repository.Please make sure you have the correct access rights and the repository exists.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-22 14:21:35 |
Closed_By | ⇒ | miguelbgouveia |
Status | Closed | ⇒ | New |
Closed_Date | 2016-08-22 14:21:35 | ⇒ | |
Closed_By | miguelbgouveia | ⇒ |
Status | New | ⇒ | Pending |
Use https://github.com/joomla/joomla-cms.git
for the address instead of the git@
address. The former allows anyone to read from a repo, the latter requires write access.
Did you enter you password correct? Maybe you can also try git pull upstream staging
If you've got your full SSH environment set up right then generally it will. For read only operations though, you really don't need it and can get away with the https
URL.
https://help.github.com/articles/which-remote-url-should-i-use/
Thanks just found it.
@miguelbgouveia is this PR still for testing?
I'm not more working with joomla CMS and I can't verify the correctness of this PR.
As this PR has been abandoned by the submitter and it is not testable or mergable I am going to have to close it at this time. It can always be reopened if updated
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-21 17:40:52 |
Closed_By | ⇒ | brianteeman |
Category | Search Administration Components Plugins Front End | ⇒ | Administration com_finder Front End Plugins Search Components |
@miguelbgouveia can you make more clear test instrutions?
like step 1, 2. 3. how it behaves before, how it behaves after?
For instance, i've never used com_finder before and have no idea what this PR is supposed to do and how to test it.