global articles are displayed #4
Conversation
There was a problem hiding this comment.
Review Summary
Thank you for your hard work! 😊 However, there are some critical requirements that are not fully met yet. Specifically, not all required test scenarios are covered, several components are not properly initialized (some code is commented out), and some class extensions and usages are missing or unconfirmed. Please address these issues—uncomment and complete all necessary initializations, ensure all subclasses are implemented and extended correctly, and add the missing test scenarios. Once these are fixed, your PR will be ready for approval! Keep going, you're almost there! 🚀
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/ui/components/ArticleListItem.js
Outdated
| constructor(page) { | ||
| super(page); | ||
| this.articleList = this.page.getByRole('link', { | ||
| name: 'Article title: consequuntur-', |
There was a problem hiding this comment.
Potential issue: The article title is hardcoded as 'consequuntur-'. If the checklist requires dynamic or parameterized selectors, you should update this to accept the title as a parameter. Otherwise, this is acceptable for a static test.
| async clickGlobalFeedTab() { | ||
| await this.step(`Click 'Global Feed' tab`, async () => { | ||
| await this.#globalFeedLink.click(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Redundancy issue: The clickGlobalFeedTab() method duplicates the functionality of the open() method. If the checklist requires avoiding duplicate methods, consider consolidating these into a single method.
src/ui/pages/home/BaseHomePage.js
Outdated
| @@ -1,10 +1,12 @@ | |||
| import { BasePage } from '../BasePage'; | |||
| import { GlobalFeedTab } from '../../components/GlobalFeedTab'; | |||
| // import { PopularTags } from '../components/PopularTags'; | |||
There was a problem hiding this comment.
Commented-out import: If the checklist requires the PopularTags component to be imported, uncomment this line.
src/ui/pages/home/BaseHomePage.js
Outdated
|
|
||
| export class BaseHomePage extends BasePage { | ||
| constructor(page, userId = 0) { | ||
| super(page, userId); | ||
| this._url = '/'; | ||
| this.globalFeed = new GlobalFeedTab(this.page, userId); | ||
| // this.popularTags = new PopularTags(page); |
There was a problem hiding this comment.
Commented-out initialization: If the checklist requires the PopularTags component to be initialized, uncomment this line.
| @@ -1,9 +1,13 @@ | |||
| import { BaseHomePage } from './BaseHomePage'; | |||
| // import { BaseViewArticlePage } from './BaseViewArticlePage'; | |||
There was a problem hiding this comment.
Commented-out import: If the checklist requires the BaseViewArticlePage component to be imported, uncomment this line.
|
|
||
| export class ExternalHomePage extends BaseHomePage { | ||
| constructor(page, userId = 0) { | ||
| super(page, userId); | ||
| this.header = new ExternalHeader(this.page, userId); | ||
| this.tagFeed = new TagFeed(page); | ||
| // this.baseViewArticlePage = new BaseViewArticlePage(page); |
There was a problem hiding this comment.
Commented-out initialization: If the checklist requires the BaseViewArticlePage component to be initialized, uncomment this line.
| @@ -1,12 +1,16 @@ | |||
| import { BaseHomePage } from './BaseHomePage'; | |||
| import { InternalHeader } from '../../components/header/InternalHeader'; | |||
| import { YourFeedTab } from '../../components/YourFeedTab'; | |||
| import { TagFeed } from '../../components/TagFeed'; | |||
| // import { BaseViewArticlePage } from './BaseViewArticlePage'; | |||
There was a problem hiding this comment.
Commented-out import: If the checklist requires the BaseViewArticlePage component to be imported, uncomment this line.
|
|
||
| export class InternalHomePage extends BaseHomePage { | ||
| constructor(page, userId = 0) { | ||
| super(page, userId); | ||
|
|
||
| this.header = new InternalHeader(this.page, userId); | ||
| this.yourFeed = new YourFeedTab(this.page, userId); | ||
| this.tagFeed = new TagFeed(page); | ||
| // this.baseViewArticlePage = new BaseViewArticlePage(page); |
There was a problem hiding this comment.
Commented-out initialization: If the checklist requires the BaseViewArticlePage component to be initialized, uncomment this line.
There was a problem hiding this comment.
Review Summary
Great work! 🎉 Your implementation meets all the core requirements and constraints of the task. All necessary components, pages, and tests are present and correctly structured. Previous issues have been addressed, and there are no critical problems remaining. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| export class ExternalHomePage extends BaseHomePage { | ||
| constructor(page, userId = 0) { | ||
| super(page, userId); | ||
| this.header = new ExternalHeader(this.page, userId); | ||
| this.tagFeed = new TagFeed(page); | ||
| // this.baseViewArticlePage = new BaseViewArticlePage(page); |
There was a problem hiding this comment.
The initialization of BaseViewArticlePage is commented out. If the checklist or task requires this component to be initialized or used, please uncomment and ensure it is properly set up.
| await internalHomePage.globalFeedTab.open(); | ||
|
|
||
| await internalHomePage.popularTags.clickTag(tagName); | ||
| await internalHomePage.popularTags.assertTagIsVisible(tagName); |
There was a problem hiding this comment.
The test currently clicks the tag and asserts the tag is visible, but it does not check that the article is actually present in the tag feed. If the task requires verifying that the article appears after filtering by tag, you should add an assertion (e.g., using internalHomePage.articleListItem.assertArticleIsVisible(article.title)) after this step.
| users, | ||
| }) => { | ||
| const viewArticlePage = new ViewArticlePage(pages[1], 2); | ||
|
|
There was a problem hiding this comment.
The test is missing a step to open the article page before asserting its content. You should call viewArticlePage.open(articleWithoutTags.url) before the assertions to ensure the page is loaded.
|
|
||
| await homePage.yourFeed.assertTabLinkVisible(); | ||
| await globalFeedTab.clickGlobalFeedTab(); | ||
| await globalFeedTab.articleListItem.assertGlobalListArticlesIsVisible(); |
There was a problem hiding this comment.
The method assertGlobalListArticlesIsVisible is called on globalFeedTab.articleListItem. Please ensure that this method exists in the ArticleListItem class. If it does not exist, you need to implement it or use a correct assertion method for verifying that articles are visible in the global feed.
No description provided.