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

Credit Memo number generator requires refactor #191

Closed
diimpp opened this issue Feb 18, 2020 · 0 comments · Fixed by #299
Closed

Credit Memo number generator requires refactor #191

diimpp opened this issue Feb 18, 2020 · 0 comments · Fixed by #299
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).

Comments

@diimpp
Copy link
Member

diimpp commented Feb 18, 2020

Hi,

There are multiple problems with given number generator, functionally, styling and idiomatic wise.

  • NumberGenerator interface's generate method doesn't have arguments, like OrderNumberGeneratorInterface's generate have $order, which leads to problem by which data to generate the number.
  • https://github.com/Sylius/RefundPlugin/blob/master/src/Generator/NumberGenerator.php NumberGenerator interface doesn't have Interface part in it's name
  • Sylius traditional placement is src/NumberGenerator, name should be CreditMemoNumberGeneratorInterface
  • SequentialNumberGenerator uses DateTime('now') as source of the date, but it should take date from CreditMemo IssuedAt. This is outright bug and was done so, because NumberGenerator interface's generate doesn't have CreditMemo as argument I've noticed how CreditMemo is instanced and why CreditMemo is not supplied to number generator, but issue still holds true: generate(OrderInterface $order, \DateTimeInterface $issuedAt) Something like this maybe?
  • Entity\SequenceInterface::getId should be nullable Done
  • Entity\SequenceInterface should be named as CreditMemoSequenceInterface, this plugin is not a place to offer generic sequence interface Done
  • Needs strict comparison Done
  • DI definition is not friendly for extension. Interface should be used as base service name or regular non-fqcn string name.

Some of the entities of this plugin, including CreditMemoSequence don't implement ResourceInterface. Is it some kind of policy? I would have preferred them to be regular resources, whenever possible.

@Zales0123 Zales0123 added DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Feb 25, 2020
@diimpp diimpp mentioned this issue Mar 6, 2020
lchrusciel added a commit that referenced this issue Apr 8, 2020
This PR was merged into the 1.0-dev branch.

Discussion
----------

Tried to collect BC changes in single PR.

Fixes #192
References #191 

- Renames SequenceInterface to CreditMemoSequenceInterface (Like OrderSequenceInterface, InvoiceSequenceInterface)
- Makes CreditMemo `number` unique and `issuedAt` non nullable (In doctrine mappings, for the rest of the app it's already holds true)
- Rename `sylius_refund_payment` to `sylius_refund_refund_payment` (Table was named incorrectly)
- Splits CustomerBillingData `customerName` into `firstName` and `lastName` (As in Addressing\Model\Address, Customer\Model\Customer, Invoicing\Entity\BillingData)
- Change CreditMemo `issuedAt` to datetime_immutable (Note, that I've converted CurrentDateTimeInterface to CurrentDateTimeImmutableInterface due to phpstan unability to handle properly \DateTimeInterface in context of \DateTime and \DateTimeImmutable phpstan/phpstan#314 https://phpstan.org/r/d15041c9-a230-4b18-a627-fc859ea00838 )

Commits
-------

36948ef Rename SequenceInterface to CreditMemoSequenceInterface
a731e22 Make CreditMemo number unique and issuedAt non nullable
055fba9 Rename sylius_refund_payment to sylius_refund_refund_payment
ac59e85 Split CustomerBillingData's customerName into firstName and lastName
33f5676 Cover edge cased with Asserts; Fix related specs
af5147b Change CreditMemo issuedAt to immutable
ed33695 Change CurrentDateTimeProviderInterface to CurrentDateTimeImmutableProviderInterface due to phpstan bug
Zales0123 added a commit that referenced this issue May 31, 2021
This PR was merged into the 1.0-dev branch.

Discussion
----------

Associated with #191

Commits
-------

8b31592 Add interface prefix to number generator
lchrusciel added a commit that referenced this issue Jun 1, 2021
This PR was merged into the 1.0-dev branch.

Discussion
----------

Fixes #191

Commits
-------

e3dc7e0 Refactor credit memo number generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants