-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fix #25761: Site map doesn't include home page #26445
Fix #25761: Site map doesn't include home page #26445
Conversation
Hi @deepaksnair. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
@magento give me test instance |
@magento give me 2.4-develop instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide automated tests to the PR?
<group id="generate" translate="label" type="text" sortOrder="4" showInDefault="1"> | ||
<group id="store" translate="label" type="text" sortOrder="4" showInDefault="1" showInWebsite="1" showInStore="1"> | ||
<label>Store Url Options</label> | ||
<field id="changefreq" translate="label" type="select" sortOrder="1" showInDefault="1" showInWebsite="1" showInStore="1" canRestore="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good practice is to increase sortOrder
numbers by 10. This way extension developers are still able to extend the grid with their elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lbajsarowicz , thanks for reveiwing PR, I have updated the sort orders. Please help to verify.
Also can you please suggest how can I provide automated tests to PR.
Hi @deepaksnair. Thank you for your request. I'm working on Magento instance for you |
Hi @deepaksnair. Thank you for your request. I'm working on Magento 2.4-develop instance for you |
Hi @deepaksnair, here is your Magento instance. |
Hello @lbajsarowicz , I have added automated test cases in last commit, please help to verify PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Only a few things to be changed.
} | ||
|
||
/** | ||
* @return \PHPUnit_Framework_MockObject_MockObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use \PHPUnit\Framework\MockObject\MockObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And return type is actually:
SitemapItemInterfaceFactory|MockObject
} | ||
|
||
/** | ||
* @return \PHPUnit_Framework_MockObject_MockObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use \PHPUnit\Framework\MockObject\MockObject
And return type is ConfigReaderInterface|MockObject
use Magento\Sitemap\Model\SitemapItem; | ||
use Magento\Sitemap\Model\SitemapItemInterfaceFactory; | ||
|
||
class StoreUrlTest extends \PHPUnit\Framework\TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import class.
$resolver = new StoreUrlItemResolver($configReaderMock, $itemFactoryMock); | ||
$items = $resolver->getItems(1); | ||
|
||
$this->assertTrue(count($items) == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is ->assertCount(1, $items)
} | ||
|
||
/** | ||
* {@inheritdoc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inheritdoc
should not be in braces
} | ||
|
||
/** | ||
* {@inheritdoc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inheritdoc
should not be in braces
Hello @lbajsarowicz , I have resolved all the feedback given by you, please help to review it again. |
Hi @lbajsarowicz, thank you for the review.
|
Hi @deepaksnair, thank you for your contribution. Could you fix failing static tests and add this phrase Thanks! |
@deepaksnair Please fix Static Tests. |
Hello @lbajsarowicz , @engcom-Alfa , I have resolved the issues in Static Tests also added translations, please help to verify. |
Hi @lbajsarowicz, thank you for the review. |
✔️ QA Passed |
Hi @deepaksnair, thank you for your contribution! |
Description (*)
Site map doesn't include home page url issue fixed
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)