Skip to content

Conversation

@dverkade
Copy link
Member

@dverkade dverkade commented Jul 9, 2018

Description

Upon investigation there is a totalbar block in the create invoice and credit memo screens which is not being used. This PR addresses that:

  • Removed the unused Totalbar block from the invoice create and credit memo create layouts.
  • Deprecated Totalbar class
  • Deprecated totalbar template file

Fixed Issues

  1. Block totalbar not used in invoice create and credit memo create screens #16655: Block totalbar not used in invoice create and credit memo create screens
  2. Not possible to create an invoice in Magento 2.3 #16653: Not possible to create an invoice in Magento 2.3

Related PR's

There is a PR related to this PR, which does not solve the issue correctly.

  1. $block->getTotals() is not countable #16563: $block->getTotals() is not countable

Manual testing scenarios

  1. Create a new order
  2. Create an invoice, screen will look the same as before the changes.
  3. Update invoice quantity, screen will look the same as before the changes.
  4. Create a credit memo, screen will look the same as before the changes.
  5. Update credit memo quantity, screen will look the same as before the changes.

…t memo create layouts.

- Deprecated Totalbar class
- Deprecated totalbar template file
@magento-engcom-team
Copy link
Contributor

Hi @dverkade. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

Thanks for comprehensive investigation @dverkade 👍

@dverkade
Copy link
Member Author

dverkade commented Aug 1, 2018

Hi @sidolov,

Could you move this PR forward? It has been reviewed already and issue #16655 has already been acknowledged, so this one is an easy fix.

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-2623 has been created to process this Pull Request

@orlangur
Copy link
Contributor

orlangur commented Aug 1, 2018

@dverkade probably, it's because mentioned issue is closed. Looks like it can be slightly reworded and reopened (block is actually used on 2.3-develop - but is useless).

@magento-engcom-team
Copy link
Contributor

Hi @dverkade. Thank you for your contribution.
We will aim to release these changes as part of 2.2.7.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Aug 8, 2018

Hi @dverkade,
In magento 2.1.x we had quite a log questions why any piece of code was deprecated and what we should use instead. I think it will be good idea to describe in for this particular case. Could you prepare PR with clarification?

@dverkade
Copy link
Member Author

dverkade commented Aug 8, 2018

Hi @ihor-sviziev,

Thanks for your comment. The funny thing is that this block has not beed used anymore for a long time, and doesn't actually render anything anymore. If it still does render something, there is no styling on the block. So if a third party vendor or a merchant has used this blocked, they would have changed it already. I don't know what this block was used for, and hasn't been used for a while. It just should not be used (anymore) by anyone, so deprecating this should be enough.

@ihor-sviziev
Copy link
Contributor

@dverkade good point, thx for clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants