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.

Sign up for GitHub

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

Jump to bottom

Fix for #9566: Show the correct label in the admin #11397

Merged
merged 7 commits into from
Nov 1, 2017
Merged

Fix for #9566: Show the correct label in the admin #11397

merged 7 commits into from
Nov 1, 2017

Conversation

michielgerritsen
Copy link
Member

Description

When creating an order status it is possible to set different labels for the different store views. This label was used in the backend as well, this fix makes sure the general status label is used in the backend.

Fixed Issues

  1. Status label is wrong in admin #9566: Status label is wrong in admin

Manual testing scenarios

  1. Create order
  2. Open edit order page in admin and note "Order Status"
  3. Go to Admin: Stores>Settings>Order Status and edit row with status that has order
  4. Change label for Default Store View = "Processing on store view"
  5. Refresh order edit form

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@dmanners dmanners self-assigned this Oct 12, 2017
@dmanners dmanners added this to the October 2017 milestone Oct 12, 2017
okorshenko
okorshenko suggested changes Oct 12, 2017
View reviewed changes
dev/tests/integration/testsuite/Magento/Sales/Model/Order/StatusTest.php
* Class ShipmentTest
* @package Magento\Sales\Model\Order
*/
class StatusTest extends \PHPUnit\Framework\TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

you can have I test + dataPorviderer instead of 2 similar tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@okorshenko are you happy with this updated test?

@michielgerritsen
Copy link
Member Author

Hi @okorshenko,

I've updated the test to use a data provider.

@okorshenko okorshenko self-assigned this Oct 17, 2017
@okorshenko okorshenko added 2.2.x bug report Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 17, 2017
@dmanners
Copy link
Contributor

Hi @michielgerritsen thanks for the PR so far it looks good and the status is changed on the main order detail display. What would your thoughts be on also updating the "Notes for this order" section's drop-down also?

order-status

@michielgerritsen
Copy link
Member Author

michielgerritsen commented Oct 25, 2017 •
edited
Loading

Hi David,

Thanks, makes sense. I just updated the PR with changes to use the correct label in that dropdown.

dmanners
dmanners suggested changes Oct 25, 2017
View reviewed changes
Copy link
Contributor

@dmanners dmanners 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 this change. It does look like there is a bit of code duplication here. Could we consider using the same method for both cases?

app/code/Magento/Sales/Model/Order/Config.php Outdated
@@ -211,7 +218,7 @@ public function getStateStatuses($state, $addLabels = true)
foreach ($collection as $item) {
$status = $item->getData('status');
if ($addLabels) {
$statuses[$status] = $item->getStoreLabel();
$statuses[$status] = $area == 'adminhtml' ? $item->getLabel() : $item->getStoreLabel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Great looks like this fixes the problem. It does appear that this is the same check as in getStatusLabel, assuming that $item here and $status in the getStatusLabel are the same type. Is it possible to use the same method for getting the label based on the area?

@michielgerritsen
Copy link
Member Author

Makes sense. I just changed the code by calling the getStatusLabel, which holds the area check.

@dmanners
Copy link
Contributor

Thanks @michielgerritsen code looks much cleaner now. Could you take a look at the unit tests that are failing. You should be able to run the failing test cases by running ./vendor/bin/phpunit --config dev/tests/unit/phpunit.xml.dist app/code/Magento/Sales/Test/Unit/Model/Order/ConfigTest.php

1) Magento\Sales\Test\Unit\Model\Order\ConfigTest::testGetStatuses with data set "pending state" ('pending', true, array(Magento\Framework\DataObject Object (...)), array('Pending label'))
Error: Call to a member function load() on null
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Config.php:127
/home/travis/build/magento/magento2/app/code/Magento/Sales/Model/Order/Config.php:220
/home/travis/build/magento/magento2/app/code/Magento/Sales/Test/Unit/Model/Order/ConfigTest.php:150

@michielgerritsen
Copy link
Member Author

Hi @dmanners,

It took some time to find time, but i just updated the code to fix the test. Fingers crossed that Travis will mark it as green :-)

@michielgerritsen
Copy link
Member Author

Does @okorshenko maybe have a little bit time to look at this on the last day of #squashtoberfest?

@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@okorshenko okorshenko merged commit 0784f58 into magento:2.2-develop Nov 1, 2017
okorshenko pushed a commit that referenced this pull request Nov 1, 2017
@ishakhsuvarov
MAGETWO-81786: Fix for #9566: Show the correct label in the admin #11397
0bcfc6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okorshenko okorshenko okorshenko requested changes

@dmanners dmanners dmanners approved these changes

Assignees

@dmanners dmanners

@okorshenko okorshenko

Labels
bug report Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Milestone
November 2017
Development

Successfully merging this pull request may close these issues.
4 participants
@michielgerritsen @dmanners @okorshenko @magento-team