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.

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

P#56183 Optimize columns on list views for forms estates and addresses #139

Conversation

tung-le-esg
Copy link
Contributor

P#56183 Optimize columns on list views for forms estates and addresses

@coveralls
Copy link

coveralls commented Dec 20, 2021

Pull Request Test Coverage Report for Build 1964212787

  • 37 of 110 (33.64%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 78.611%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugin/Record/RecordManagerReadListViewEstate.php 0 6 0.0%
plugin/Controller/DetailViewPostSaveController.php 32 99 32.32%
Files with Coverage Reduction New Missed Lines %
plugin/Controller/DetailViewPostSaveController.php 2 35.66%
Totals Coverage Status
Change from base Build 1962442938: -0.1%
Covered Lines: 6123
Relevant Lines: 7789

💛 - Coveralls

@tung-le-esg tung-le-esg requested a review from jayay December 20, 2021 03:54
@@ -213,7 +214,7 @@ public function testUninstall(array $createQueries)
// assert that as many tables have been removed as have been created
$uniqueCreateQueries = array_unique($createQueries);
$uniqueDropQueries = array_unique($this->_dropQueries);
$this->assertEquals(count($uniqueCreateQueries), count($uniqueDropQueries));
$this->assertGreaterThanOrEqual(count($uniqueCreateQueries), count($uniqueDropQueries));
Copy link
Contributor

Choose a reason for hiding this comment

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

How can more tables be dropped than created?

}
if (strpos($view->page_shortcode,(string)$postID)!== false)
{
$pageID = chop($view->page_shortcode,$postID);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks unstable. Better use explode, array functions. and implode.

@@ -144,7 +174,9 @@ public function prepare_items()
$columns = [
'cb' => '<input type="checkbox" />',
'name' => __('Name of View', 'onoffice-for-wp-websites'),
'template' => __('Tempaltes', 'onoffice-for-wp-websites'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in "Templates".

@@ -119,6 +124,31 @@ private function fillData()
]);
}

private function handleRecord($listRecord)
Copy link
Contributor

Choose a reason for hiding this comment

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

This block was duplicated so often. Can it be factored out into something reusable?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still open

$page = '';
foreach ($listPageID as $pageID)
{
if (!empty($page))
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use implode.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is still open, too

{
$page .= ',';
}
$page .= "<a href='".get_edit_post_link((int)$pageID)."' target='_blank'>".get_the_title((int)$pageID)."</a>";
Copy link
Contributor

Choose a reason for hiding this comment

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

esc_attr() missing around get_edit_post_link() and esc_html() missing around get_the_title.

{
return [];
}
foreach ($listRecord as &$record)
Copy link
Contributor

Choose a reason for hiding this comment

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

An array of new records would be cleaner than &.

@@ -274,6 +281,7 @@ private function getCreateQueryForms()
`radius` INT( 10 ) NULL DEFAULT NULL,
`geo_order` VARCHAR( 255 ) NOT NULL DEFAULT 'street,zip,city,country,radius',
`show_estate_context` tinyint(1) NOT NULL DEFAULT '0',
`page_shortcode` tinytext NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this duplicate information? This is the name with some constant string left and right. Totally reproducible.

@vu-vuong-esg vu-vuong-esg requested a review from jayay January 4, 2022 02:55
@@ -119,6 +124,31 @@ private function fillData()
]);
}

private function handleRecord($listRecord)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still open

$page = '';
foreach ($listPageID as $pageID)
{
if (!empty($page))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is still open, too

@vu-vuong-esg vu-vuong-esg force-pushed the 56183-Optimize-columns-on-list-views-for-forms-estates-and-addresses branch from c0347a8 to 500acc0 Compare January 12, 2022 01:58
@vu-vuong-esg vu-vuong-esg requested a review from jayay January 12, 2022 09:47
@nglelinh nglelinh removed the request for review from jayay January 26, 2022 09:14
@michaelarnold-dev
Copy link
Contributor

@hungnc89 test feedback "Test with version 2.21.4-13-gd93dcbf6 installed on https://wpptest.onofficeweb.com/. See also the attached video.

Errors.mp4

1.) After installing the project code all previously created lists, forms etc. are gone. When switching back to another code, they are there again.
2.) After saving a list or a form it does not appear in list view.
3.) The names from the screenshots are used everywhere. But the names from the text in the requirements are the relevant ones. Only for forms instead of "List name" is better "Form name"."

@tang-hien-egs
Copy link
Contributor

@hungnc89 test feedback "Test with version 2.21.4-13-gd93dcbf6 installed on https://wpptest.onofficeweb.com/. See also the attached video.

Errors.mp4
1.) After installing the project code all previously created lists, forms etc. are gone. When switching back to another code, they are there again. 2.) After saving a list or a form it does not appear in list view. 3.) The names from the screenshots are used everywhere. But the names from the text in the requirements are the relevant ones. Only for forms instead of "List name" is better "Form name"."

@michaelarnold-dev , i tested in localhost oki, i recorded in video like link : https://files.fm/f/87gz62b92, can you check and upgrade db_version ?

@michaelarnold-dev
Copy link
Contributor

@tang-hien-egs Robert said that 3) is still open. He asks you to consider these values /fields

1.1) Add the following columns to the estate list overview:

  • List name
  • Selected filter
  • Template
  • List type
  • Shortcode
  • Pages using the shortcode

1.2) For the forms:

  • Form name
  • Recipient e-mail address
  • Template
  • Form type
  • Shortcode
  • Pages using the shortcode

1.3) For addresses:

  • List name
  • Selected filter
  • Template
  • List type
  • Shortcode
  • Pages using the shortcode

@tang-hien-egs
Copy link
Contributor

@tang-hien-egs Robert said that 3) is still open. He asks you to consider these values /fields

1.1) Add the following columns to the estate list overview:

  • List name
  • Selected filter
  • Template
  • List type
  • Shortcode
  • Pages using the shortcode

1.2) For the forms:

  • Form name
  • Recipient e-mail address
  • Template
  • Form type
  • Shortcode
  • Pages using the shortcode

1.3) For addresses:

  • List name
  • Selected filter
  • Template
  • List type
  • Shortcode
  • Pages using the shortcode

@michaelarnold-dev , i fixed it, can you check it ?

@tang-hien-egs
Copy link
Contributor

@michaelarnold-dev , i tested local and record video like link : https://files.fm/f/yqk2a5zwr

@jayay
Copy link
Contributor

jayay commented Mar 10, 2022

@tang-hien-egs could you please clean up the code from your last commit?

@tang-hien-egs
Copy link
Contributor

@tang-hien-egs could you please clean up the code from your last commit?

@jayay i fixed it and fixed conflict, can you check PR help me ?

@michaelarnold-dev michaelarnold-dev merged commit 7f029b8 into master Mar 15, 2022
@jayay jayay deleted the 56183-Optimize-columns-on-list-views-for-forms-estates-and-addresses branch March 15, 2022 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants