(PC-1501): refactoring routes offers things events #500

sofcalca · 2019-03-20T15:21:28Z

No description provided.

abel-andre · 2019-03-21T15:05:47Z

alembic/versions/5eeaa2f48327_copy_thing_and_event_attributes_to_offer.py

+    op.add_column('offer', sa.Column('conditions', sa.String(120), nullable=True))
+    op.add_column('offer', sa.Column('ageMin', sa.Integer, nullable=True))
+    op.add_column('offer', sa.Column('ageMax', sa.Integer, nullable=True))
+    op.add_column('offer', sa.Column('accessibility', sa.Binary, nullable=True))


je pense qu'on peut ne pas récupérer cette column qui est inutile

alembic/versions/5eeaa2f48327_copy_thing_and_event_attributes_to_offer.py

abel-andre · 2019-03-21T15:16:22Z

alembic/versions/5eeaa2f48327_copy_thing_and_event_attributes_to_offer.py

+    op.alter_column('offer', 'isNational', nullable=False)
+
+
+def downgrade():


Besoin de drop la constraint check_duration_minutes_not_null_for_event ou non ?

comme la colonne disparait, pour moi elle part seule, non ?

Aucune idée, je sais qu'on en avait discuté la dernière fois pendant le mob mais je me souviens plus de la conclusion :-(

abel-andre · 2019-03-21T15:17:05Z

alembic/versions/ddf0dc458d57_add_author_to_thing_and_event.py

@@ -0,0 +1,25 @@
+"""add authorId to event and thing


Attention à remplacer 'author_id' ici et dans le nom du fichier par owning_offerer_id

abel-andre · 2019-03-21T15:55:31Z

tests/routes/patch_offer_test.py

+            assert thing.name == 'New Name'
+
+        @clean_database
+        def when_user_updating_thing_offer_is_not_linked_to_owningOfferers_offerers(self, app):


typo dans le nom du test ?

abel-andre · 2019-03-21T15:56:30Z

tests/routes/patch_offer_test.py

+
+    class Returns400:
+        @clean_database
+        def when_trying_to_patch_forbidden_keys(self, app):


forbidden attributes plutôt ?

abel-andre · 2019-03-21T16:01:47Z

validation/offers.py

@@ -20,3 +22,17 @@ def check_venue_exists_when_requested(venue, venue_id):
            "Ce lieu n'a pas été trouvé"
        )
        raise errors
+
+
+def check_valid_edition(response: Request, thing_or_event_dict: dict):


attention, ce n'est pas une response mais une request !

abel-andre · 2019-03-21T16:04:40Z

tests/routes/post_offer_test.py

+
+@pytest.mark.standalone
+class Post:
+    class Returns400:


Je pense que certains de ces cas d'erreurs sont testés plus bas dans le code au niveau de Offer.errors(), donc est-ce qu'il faut tous les tester ici ? Comment choisir lesquels on teste au niveau de la route ?

Si t'as un avis sur la question... je pense qu'on manque d'une règle explicite là dessus

En fait au niveau des erreurs on teste le fait qu'on lève bien l'exception ApiErrors. Ensuite, il me semble important de tester que quand on a une ApiError, notre route renvoie bien une 400. L'idéal à mon avis serait d'avoir un test qui dit "quand on a un ApiError, la route renvoie 400", mais on ne peut pas (encore) mocker la réponse des checks 😢
Une autre option c'est de ne garder qu'un cas d'erreur 400 pour être sûrs que cette exception est bien traitée dans les error handlers. Qu'en penses-tu ?

Une autre option c'est de ne garder qu'un cas d'erreur 400 pour être sûrs que cette exception est bien traitée dans les error handlers

Je vote pour ça ! C'est l'approche qu'on avait eu la semaine dernière avec Anthony sur une autre PR

abel-andre · 2019-03-21T16:07:08Z

tests/routes/post_offer_test.py

+            event = Event.query.filter_by(id=event_id).first()
+            assert event.durationMinutes == 60
+            assert event.name == 'La pièce de théâtre'
+            assert offer.type == str(EventType.SPECTACLE_VIVANT)


Ça fait beaucoup d'assertions d'un coup dans tout ces tests. Possible de découper ?

Ce qui me gêne de découper c'est que :

ça fait tourner plus de tests de route, qui sont les plus longs

dans le cas d'usage "doc API" ça pollue un peu

Mais c'est vrai que pour la lecture des tests c'est plus facile.

Tu en penses quoi ?

annemarie35 · 2019-03-22T08:55:32Z

models/offer.py

        if self.venue:
            venue = self.venue
        else:
            venue = Venue.query.get(self.venueId)
-        if thing and thing.url and not venue.isVirtual:
+        if self.isDigital and not venue.isVirtual:


…vent

…gs and events

… change in table constraints

…telive stocks

…isplaced

Ledoux · 2019-04-08T15:05:57Z

gros boulot, yes!

sofcalca · 2019-04-08T16:09:56Z

domain/create_offer.py

+    offer.conditions = event.conditions
+    offer.ageMin = event.ageMin
+    offer.ageMax = event.ageMax
+    offer.accessibility = event.accessibility


à enlever : on voulait enlever cet attribut. Il n'est pas dans la migration d'ailleurs

sofcalca · 2019-04-08T16:10:12Z

models/offer.py

+    ageMax = Column(Integer,
+                    nullable=True)
+
+    accessibility = Column(Binary(1),


Il est encore là, il faut le supprimer !

sofcalca · 2019-04-08T16:10:25Z

models/offer.py

+
+    accessibility = Column(Binary(1),
+                           CheckConstraint('("eventId" IS  NULL) OR (accessibility IS NOT NULL)',
+                                           name='check_accessibility_not_null_for_event'),


enlever la contrainte aussi

sofcalca force-pushed the PC-1501-refactoring-routes-offers-things-events branch from ddb37ef to 13215a4 Compare March 20, 2019 16:40

annemarie35 self-requested a review March 21, 2019 14:45

abel-andre reviewed Mar 21, 2019

View reviewed changes

annemarie35 reviewed Mar 22, 2019

View reviewed changes

sofcalca force-pushed the PC-1501-refactoring-routes-offers-things-events branch from 1270101 to dafcb22 Compare March 22, 2019 16:44

sofcalca force-pushed the PC-1501-refactoring-routes-offers-things-events branch from dafcb22 to b2afc03 Compare April 1, 2019 08:13

Ledoux changed the title ~~Pc 1501 refactoring routes offers things events~~ [WIP] (pC-1501) refactoring routes offers things events Apr 4, 2019

Akhilian force-pushed the PC-1501-refactoring-routes-offers-things-events branch from 4b7dfd9 to 8f1b487 Compare April 8, 2019 09:43

sofcalca and others added 22 commits April 8, 2019 11:44

(PC-1501) change model offer

69a6327

(PC-1501) migrated post from things and events to offers

3945519

(PC-1501) patching offer with bookingEmail

6d69515

(PC-1501) patching offers and related things and events

c4deff5

(PC-1501) search now looks in Offer attributes instead of Thing and E…

e9433e3

…vent

(PC-1501) removed useless code and tests linked to post and past thin…

d10425e

…gs and events

(PC-1501) correcting tests after change in tables used for search and…

7b3bb5d

… change in table constraints

(PC-1501) add new offer fields in updateObject from local provider ti…

2feb9e5

…telive stocks

(PC-1501) implemented pre-existing tests on patch offers that where m…

edaf991

…isplaced

(PC-1501) reincorporated preexisting tests for post offers

5a85e41

(PC-1501) added type hint

8fbf997

(PC-1501) renamed check constraints to correct build bug

f1da0ee

(PC-1501) added tests on offer errors

d415376

(PC-1501) corrected tests linking physical offers to virtual venues

fe0bf87

(PC-1501) correct alembic migration

c08b42f

(PC-1501) after comment reviews

13be2a4

(PC-1501): Type in routes

a9318ef

(PC-1501) Fix alembic multi head history

af6c605

(PC-1501): Rewritting e2e tests to match the new request format

222b1bd

(PC-1501): Update expected payload on offer creation

6a19452

(PC-1501) set owningOfferer when creating thing or event

35cd3f4

(PC-1501) adapted expected payload on update

06c1689

Akhilian force-pushed the PC-1501-refactoring-routes-offers-things-events branch from 8f1b487 to 06c1689 Compare April 8, 2019 09:44

Akhilian changed the title ~~[WIP] (pC-1501) refactoring routes offers things events~~ (PC-1501): refactoring routes offers things events Apr 8, 2019

Ledoux mentioned this pull request Apr 8, 2019

(PC-1501): Refactoring routes offers things events pass-culture/pass-culture-pro#301

Merged

Ledoux self-requested a review April 8, 2019 15:05

Ledoux approved these changes Apr 8, 2019

View reviewed changes

sofcalca commented Apr 8, 2019

View reviewed changes

(PC-1501) remove accessibility from offer

cd88de7

sofcalca merged commit a502b5c into master Apr 9, 2019

sofcalca deleted the PC-1501-refactoring-routes-offers-things-events branch April 9, 2019 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PC-1501): refactoring routes offers things events #500

(PC-1501): refactoring routes offers things events #500

sofcalca commented Mar 20, 2019

abel-andre Mar 21, 2019

sofcalca Mar 22, 2019

abel-andre Mar 21, 2019

sofcalca Mar 21, 2019

abel-andre Mar 22, 2019

abel-andre Mar 21, 2019

sofcalca Mar 22, 2019

abel-andre Mar 21, 2019

abel-andre Mar 21, 2019

abel-andre Mar 21, 2019

abel-andre Mar 21, 2019

sofcalca Mar 22, 2019

abel-andre Mar 22, 2019 •

edited

Loading

abel-andre Mar 21, 2019

sofcalca Mar 22, 2019

annemarie35 Mar 22, 2019

Ledoux commented Apr 8, 2019

sofcalca Apr 8, 2019

sofcalca Apr 8, 2019

sofcalca Apr 8, 2019

		op.alter_column('offer', 'isNational', nullable=False)


		def downgrade():

(PC-1501): refactoring routes offers things events #500

(PC-1501): refactoring routes offers things events #500

Conversation

sofcalca commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abel-andre Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ledoux commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abel-andre Mar 22, 2019 •

edited

Loading