Pc 1647 merge event occurrence into stock #491

abel-andre · 2019-03-15T16:36:32Z

No description provided.

repository/booking_queries.py

annemarie35 · 2019-03-20T13:28:50Z

models/offer.py

-                                   self.eventOccurrences)))
-        else:
-            return []
-
    @property
    def lastStock(self):
        query = Stock.queryNotSoftDeleted()


Pour ma compréhension du code (cf les fichiers contributing), queryNotSoftDeleted() ne devrait-il pas être dans repository/stock_queries.py ? Car là, il est dans /models/stock.py

Théoriquement si, il faudrait que cette query soit dans un module de repository. Mais ça poserait un problème de dépendence circulaire:

le package models dépendrait du package repository

le package repository dépendrait du package models

le package models dépendrait du package repository

💥

Donc en l'état on ne sait pas faire :/

repository/offer_queries.py

tests/repository_keywords_queries_test.py

sofcalca · 2019-03-20T13:09:32Z

routes/stocks.py

@@ -29,18 +31,6 @@
 ]


-def query_stocks(ts_query):


pourquoi on l'a enlevé ?

Parce que c'était inutilisé il me semble

sofcalca · 2019-03-20T13:14:57Z

validation/stocks.py

+        raise ApiErrors({'offerId': ['Ce paramètre est obligatoire']})
+
+
+def check_new_stock_has_dates(request_data: dict, offer: Offer):


je ne suis pas convaincue par le nommage : vu les autres fonctions qu'on a nommées ainsi, on s'attend à avoir une erreur si le stock n'a pas de date, alors que ce n'est pas le cas. Proposition : check_new_stock_has_dates_or_is_thing

ce que je comprends c'est un truc du genre check_if_stock_can_have_dates ?

Selon moi check_new_stock_has_dates est une intention. Le fait que ça ne lève pas d'erreur dans le cas d'une Thing est un cas particulier de cette règle mais qui respecte l'intention.

@sofcalca dans ta proposition on fait apparaître le cas particulier dans le nom, je ne suis pas pour ça parce qu'avec ce système de nommage on ne manipule plus des abstractions / intentions mais des détails d'implémentation

@annemarie35 dans le fond ta proposition me convient mais son utilisation donnerait : if check_if_stock_can_have_dates(...): et je trouve que ça se lit moyennement bien

et un check_stock_can_have_dates ?

Vu avec Sofia : on prend check_dates_are_allowed_on_new_stock

sofcalca · 2019-03-20T13:16:26Z

routes/stocks.py

    stock_queries.save_stock(new_stock)
    return jsonify(new_stock._asdict()), 201


+# TODO: Si changement d'horaires et qu'il y a des réservations il faut envoyer des mails !
+# TODO: Interdire la modification d'évenements passés


plutôt en faire des tickets non ? Pas super fan des todo dans le code

En effet on a récupéré les TODO d'un autre module sans se poser la question.
@nfournier tu peux checker si on a toujours ces besoins ?

sandboxes/scripts/creators/industrial/create_industrial_bookings.py

validation/stocks.py

tests/routes/recommendations_test.py

sofcalca · 2019-03-20T13:44:29Z

tests/routes/event_occurrences_test.py

-
-@clean_database
-@pytest.mark.standalone
-def test_route_delete_event_occurrence_deletes_event_occurrence_stocks_and_cancels_bookings(app):


Il n'y a pas de tests dans ce sens dans stocks : il faudrait probablement les rajouter pour tester que les réservations sont bien annulées

Je rajoute ça

tests/routes/export_test.py

tests/routes/stocks_test.py

sofcalca · 2019-03-21T14:59:48Z

tests/repository_keywords_queries_test.py

@@ -11,7 +11,7 @@

 @clean_database
 @pytest.mark.standalone
-def skip_test_get_keywords_analyzer(app):
+def test_get_keywords_analyzer(app):


il me semble très étrange ce test... il ne serait pas mieux de le supprimer ?

abel-andre force-pushed the PC-1647-merge-event-occurrence-into-stock branch 2 times, most recently from ea3b373 to 9453a62 Compare March 19, 2019 10:23

annemarie35 self-requested a review March 20, 2019 12:57