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

[ZEP-13] corrige le comportement d'Elasticsearch #4262

Merged

Conversation

pierre-24
Copy link
Member

@pierre-24 pierre-24 commented Apr 2, 2017

Q R
Type de modification correction de bug
Ticket(s) (issue(s)) concerné(s) #3993 #4133 #4231

QA

Cette QA nécessite Elasticsearch (mais je rappelle que c'est très facile à installer ;) ).

  • Pour le bug de transaction, si Travis passe, c'est déjà gagné :)
  • Ensuite, créer une tribune A ayant pour titre "banane verte", la publier et lancer l'indexation (make index-all). Faire une recherche sur le mot "banane" et vérifier que la tribune apparait bien.
  • Créer une tribune B ayant pour titre "banane jaune".
  • Créer un article C ayant pour titre "banane violette" et le publier, le publier et réindexer (make index-flagged). Faire une recherche sur le mot "banane" et vérifier que tout y est (deux tribunes et un article). Vérifier que l'article apparait bien premier.
  • Mettre la tribune B ("banane jaune", parce que c'est quand même meilleur) sur la page d'acceil et réindexer (make index-flagged). Faire une recherche sur le mot "banane" et vérifier que "banane jaune" apparait bien en second.
  • Retirer la tribune B de la page d'acceil, mettre la tribune A sur la page d'acceil et réindexer (make index-flagged). Faire une recherche sur le mot "banane" et vérifier que "banane jaune" apparait bien en troisième.
  • Faites vous un milkshake à la banane.
  • Ça fonctionne !
  • Code relu et approuvé !

@pierre-24 pierre-24 changed the title Fix es transaction zep 13 [ZEP-13] corrige le comportement d'Elasticsearch dans les tests unitaires Apr 2, 2017
@coveralls
Copy link

coveralls commented Apr 2, 2017

Coverage Status

Coverage increased (+16.0%) to 88.644% when pulling 21cf7ef on pierre-24:fix_ES_transaction_ZEP-13 into fb51eb7 on zestedesavoir:zep-13-b.

@vhf vhf requested a review from artragis April 2, 2017 14:34
@pierre-24 pierre-24 changed the title [ZEP-13] corrige le comportement d'Elasticsearch dans les tests unitaires [ZEP-13] corrige le comportement d'Elasticsearch Apr 2, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-17.3%) to 55.33% when pulling 22b918f on pierre-24:fix_ES_transaction_ZEP-13 into fb51eb7 on zestedesavoir:zep-13-b.

@coveralls
Copy link

coveralls commented Apr 2, 2017

Coverage Status

Coverage increased (+16.0%) to 88.643% when pulling 22b918f on pierre-24:fix_ES_transaction_ZEP-13 into fb51eb7 on zestedesavoir:zep-13-b.

@coveralls
Copy link

coveralls commented Apr 2, 2017

Coverage Status

Coverage increased (+16.0%) to 88.646% when pulling 83bf3b2 on pierre-24:fix_ES_transaction_ZEP-13 into fb51eb7 on zestedesavoir:zep-13-b.

Copy link
Member

@artragis artragis left a comment

Choose a reason for hiding this comment

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

C'est une super PR !

J'ai fait deux/trois retour sur des broutilles mais là c'est juste génial.

@@ -30,6 +30,11 @@ <h3 class="content-title" itemprop="itemListElement">
{% trans "Article publié" %}
{% elif search_result.content_type == 'TUTORIAL' %}
{% trans "Tutoriel publié" %}
{% elif search_result.content_type == 'OPINION' %}
{% trans "Opinion" %}
Copy link
Member

Choose a reason for hiding this comment

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

Le mot de base, c'est "Billiet".

{% elif search_result.content_type == 'OPINION' %}
{% trans "Opinion" %}
{% if search_result.picked %}
{% trans "mise en avant" %}
Copy link
Member

Choose a reason for hiding this comment

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

Du coup "mis en avant".

index_manager.delete_document(instance)
index_manager.refresh_index()

if index_manager.index_exists:
Copy link
Member

Choose a reason for hiding this comment

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

vue la répétition de ces trois lignes, créer une méthode delete_and_refresh peut être utile non?



def get_django_indexable_objects():
"""Return all indexable objects registered in Django"""
return [model for model in apps.get_models() if issubclass(model, AbstractESDjangoIndexable)]


class NeedIndex(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

Juste ajouter une petite docstring pour faire beau dans la doc :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done ;)

'filter': Match(content_type='ARTICLE'),
'weight': settings.ZDS_APP['search']['boosts']['publishedcontent']['if_article']
},
{
'filter': Match(content_type='OPINION'),
Copy link
Member

Choose a reason for hiding this comment

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

j'aime ce boost

Copy link
Member Author

Choose a reason for hiding this comment

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

<3

@coveralls
Copy link

coveralls commented Apr 2, 2017

Coverage Status

Coverage increased (+16.0%) to 88.646% when pulling 11828c2 on pierre-24:fix_ES_transaction_ZEP-13 into fb51eb7 on zestedesavoir:zep-13-b.

@artragis artragis merged commit 7fdbb82 into zestedesavoir:zep-13-b Apr 2, 2017
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.

3 participants