-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Get existing page data only on edit page #39746
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
base: 2.4-develop
Are you sure you want to change the base?
Get existing page data only on edit page #39746
Conversation
Hi @Mohamed-Asar. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
@magento run all tests |
@magento run all tests |
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.
Hello @Mohamed-Asar,
Thanks for the contribution!
The changes seems good to us, but please fixed the failed static test.
Thanks
@engcom-Hotel Static test failure is not related to this PR changes ? do you want me to fix that ? |
Hello @Mohamed-Asar, To proceed further with the PR, we need a green build, and for this you need to fix the static test failure. Please let us know if you have any query regarding that. Thanks |
Sure @engcom-Hotel, Looks like we need to refactor the class, i will fix and let you know |
…into issue/39743-cms-page-error-log-fix
Hello @Mohamed-Asar, Have you got a chance to look into this? Thanks |
@engcom-Hotel i have started working on it, will let you know once done |
@magento run all tests |
@magento run all tests |
@engcom-Hotel Fixed the static test, please review |
@magento run all tests |
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.
Hello @Mohamed-Asar,
Thanks for the updates!
The changes looks good to us, but some minor changes required as below. Please look into the review comments.
Thanks
private PageRepositoryInterface $pageRepository, | ||
private PageFactory $pageFactory, | ||
private CustomLayoutManagerInterface $customLayoutManager, |
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.
I guess all properties can be readonly.
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.
@engcom-Hotel added readonly
/** | ||
* Cms Page Service Class | ||
*/ | ||
class PageService |
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.
- PageService can be unit tested for the logic of getPageById, createPageFactory, and fetchAvailableCustomLayouts.
- Integration tests could ensure data provider works as expected in edit page scenarios.
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.
@engcom-Hotel i have added unit test case, please review
@magento run all tests |
Hi @Mohamed-Asar, Thanks for the collaboration & contribution! ✔️ QA PassedPreconditions:
Steps to reproduce
Before: ✖️
After: ✔️ system.log is empty Builds are failed. Hence, moving this PR to Extended Testing. Thanks. |
@magento run all tests |
@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE |
The consistent test failures for Functional B2B are known Issues and JIRA is raised for them. Other failures are inconsistent and seems to be flaky. They neither part of PR nor failing because of the PR changes. ![]() Known Issues: The consistent test failure for Functional CE is known Issues and JIRA is raised for the same. Other failure is inconsistent and seems to be flaky. They neither part of PR nor failing because of the PR changes. Known Issues: The consistent test failure for Functional EE is known Issues and JIRA is raised for the same. Other failure is inconsistent and seems to be flaky. They neither part of PR nor failing because of the PR changes. Known Issues: Hence moving this PR in Merge In Progress. |
Description (*)
Get Page data only on the edit page
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)