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

Implement order events #4018

Merged
merged 33 commits into from
May 7, 2019

Conversation

NyanKiyoshi
Copy link
Member

@NyanKiyoshi NyanKiyoshi commented Apr 25, 2019

  • For the gql type of OrderEvent we have to include:
    • New fields for getting data from the JSON parameters. [2]
    • A payment method field taken from the parameters.
    • Tests.
  • The OrderEvent model should have:
    • A field user containing who triggered the event. [4]
    • (outside) should contain a bunch of functions taking data to generate event(s)
  • Update dashboard 1.0 to be compatible with the latest changes (only order side)

  • Update docs definitions

  • Document the different parameters generated by the order events (docstrings + sphinx-apidoc)

  • Update definitions (migrations & GQL)

  • Introduce new order event types (OrderEventsEnum):

    • the order was placed
    • new: the order payment failed [failure event]
      • Implement it
    • the payment was captured (+ amount)
    • a note was added to the order
    • the order confirmation has been set
    • new: digital link was sent to the customer -> saleor/order/emails.py:46
      • Implement it
    • Manual staff actions events:
      • the tracking email was sent -> saleor/dashboard/order/views.py:688
      • new: a draft was created
        • Implement it
      • a draft was placed
      • new: added products (store a strings)
        • Implement it
      • new: removed products (idem)
        • Implement it
      • order was manually fulfilled
      • order was manually marked as paid
  • An event should be called to be created and may create other events.
  • We need to migrate the old events to the new model.
  • Refactor logic to commit on the go instead of waiting on the user's green light.

[2] Allows to retrieve additional data from an event (e.g., an amount, a quantity, a message, etc.)
[4] We want to know who triggered the event, even if it's the customer itself or a staff user.

[!] Failed payment expected data (payment id, etc. for user support) https://i.imgur.com/ayYrCjr.png

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. The code is documented (docstrings, project documentation).
  8. GraphQL schema and type definitions are up to date.
  9. Changes are mentioned in the changelog.

Magic Final Words

Closes #2443.

@NyanKiyoshi NyanKiyoshi added in progress graphql Issues related to the GraphQL API labels Apr 25, 2019
@NyanKiyoshi NyanKiyoshi self-assigned this Apr 25, 2019
@NyanKiyoshi NyanKiyoshi marked this pull request as ready for review April 26, 2019 11:50
@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0c22560). Click here to learn what that means.
The diff coverage is 92.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4018   +/-   ##
=========================================
  Coverage          ?   91.38%           
=========================================
  Files             ?      280           
  Lines             ?    15169           
  Branches          ?     1481           
=========================================
  Hits              ?    13862           
  Misses            ?      910           
  Partials          ?      397
Impacted Files Coverage Δ
saleor/core/views.py 69.44% <0%> (ø)
saleor/graphql/shipping/enums.py 100% <100%> (ø)
saleor/events/models/__init__.py 100% <100%> (ø)
saleor/graphql/order/bulk_mutations/orders.py 96.29% <100%> (ø)
saleor/graphql/order/resolvers.py 82.92% <100%> (ø)
saleor/graphql/order/mutations/draft_orders.py 95.79% <100%> (ø)
saleor/events/__init__.py 100% <100%> (ø)
saleor/payment/utils.py 92.94% <100%> (ø)
saleor/checkout/utils.py 82.49% <100%> (ø)
saleor/graphql/checkout/mutations.py 93.35% <100%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c22560...c44aa7c. Read the comment docs.

@NyanKiyoshi NyanKiyoshi force-pushed the gql/feature/order-events branch 3 times, most recently from f2c65a0 to a3b6f58 Compare April 29, 2019 10:47
@NyanKiyoshi NyanKiyoshi changed the title Implement order and some customer events Implement order events Apr 29, 2019
@NyanKiyoshi
Copy link
Member Author

I decided to move the customer events to #4039 as it would make this PR way too big, thus even harder to review.

saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/events/models/OrderEvent.py Outdated Show resolved Hide resolved
saleor/order/emails.py Outdated Show resolved Hide resolved
@NyanKiyoshi NyanKiyoshi force-pushed the gql/feature/order-events branch 2 times, most recently from 7f11133 to 81e1885 Compare April 30, 2019 14:18
@auvipy
Copy link

auvipy commented May 2, 2019

Should I expect this feature on April release?

@NyanKiyoshi
Copy link
Member Author

@maarcingebala could you double check the OrderEvent model definitions? I'm confused by how the choices are supposed to be populated.

For example, OrderStatus populates the choices with [('status-type', 'status-type-human-readable-through-gettext')], in the database the human readable value is stored instead of the "key". So I'm confused how we should actually proceed with choices, especially as the value can be different depending on what gettext() returned.

But it may work just fine, and seems to be the proper django way. I'm unsure about how django is performing the queries and returning the results, I can't seem to find any piece of documentation on that matter.

# note this won't work until
# https://github.com/graphql-python/graphene/issues/956 is fixed
deprecation_reason = getattr(enum_cls, '__deprecation_reason__', None)
if deprecation_reason:
Copy link
Member Author

Choose a reason for hiding this comment

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

Careful here, its not supposed to be actually in a if statement but a simple setdefault. This was made like that because it would otherwise actually crash. For that reason it would be good to wait for the graphql-python/graphene#957 fix to be merged.

@NyanKiyoshi NyanKiyoshi force-pushed the gql/feature/order-events branch 2 times, most recently from 43daed1 to c44aa7c Compare May 6, 2019 11:17
@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@6ba9f78). Click here to learn what that means.
The diff coverage is 94.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4018   +/-   ##
=========================================
  Coverage          ?   91.55%           
=========================================
  Files             ?      277           
  Lines             ?    15106           
  Branches          ?     1474           
=========================================
  Hits              ?    13830           
  Misses            ?      878           
  Partials          ?      398
Impacted Files Coverage Δ
saleor/order/events.py 100% <100%> (ø)
saleor/graphql/checkout/mutations.py 93.35% <100%> (ø)
saleor/order/models.py 89.62% <100%> (ø)
saleor/graphql/payment/enums.py 90% <100%> (ø)
saleor/graphql/shipping/enums.py 100% <100%> (ø)
saleor/graphql/account/enums.py 100% <100%> (ø)
saleor/graphql/order/bulk_mutations/orders.py 95.83% <100%> (ø)
saleor/graphql/order/resolvers.py 82.92% <100%> (ø)
saleor/graphql/order/mutations/draft_orders.py 95.75% <100%> (ø)
saleor/graphql/order/types.py 84.31% <100%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ba9f78...03ad801. Read the comment docs.

@auvipy
Copy link

auvipy commented May 6, 2019

seems another rebase needed?

saleor/checkout/utils.py Outdated Show resolved Hide resolved
docs/architecture/events.rst Outdated Show resolved Hide resolved
saleor/checkout/utils.py Outdated Show resolved Hide resolved
saleor/checkout/utils.py Outdated Show resolved Hide resolved
saleor/events/bases.py Outdated Show resolved Hide resolved
@@ -11,6 +11,35 @@ class ReportingPeriod(graphene.Enum):
THIS_MONTH = 'THIS_MONTH'


def to_enum(enum_cls, *, type_name=None, **options) -> graphene.Enum:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this function and what were the consequences of not having it before? Are there any problems with enums currently?

Copy link
Member Author

@NyanKiyoshi NyanKiyoshi May 6, 2019

Choose a reason for hiding this comment

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

It avoids to repetitively have around each graphql package:

graphene.Enum('something', 
    str_to_enum(code.upper()), code) for code, name in enum_cls.CHOICES)

It would make it more readable and easier to maintain.

saleor/events/__init__.py Outdated Show resolved Hide resolved
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

great job.

@maarcingebala maarcingebala merged commit e3af84f into saleor:master May 7, 2019
@maarcingebala
Copy link
Member

🎉 Thanks @NyanKiyoshi, great effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphql Issues related to the GraphQL API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log order and customer actions and events
4 participants