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#48327(Task) Make contact selectable #88

Merged
merged 27 commits into from
Mar 8, 2022

Conversation

hungnc89
Copy link
Contributor

No description provided.

@hungnc89 hungnc89 requested a review from jayay June 24, 2021 05:00
…806-make-contact-selectable

� Conflicts:
�	plugin/Installer/DatabaseChanges.php
@jayay
Copy link
Contributor

jayay commented Jul 26, 2021

Thanks! Is it possible to cover more lines with tests?

@hungnc89
Copy link
Contributor Author

Hi @jayay,
I added tests for code.
Thanks

@jayay
Copy link
Contributor

jayay commented Jul 30, 2021

@hungnc89 Thank you so much! The test is broken, would you please have a look?

@hungnc89
Copy link
Contributor Author

@jayay oh sorry. I fixed it

Copy link
Contributor

@jayay jayay left a comment

Choose a reason for hiding this comment

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

Thank you!

@coveralls
Copy link

coveralls commented Oct 27, 2021

Pull Request Test Coverage Report for Build 1920824450

  • 40 of 41 (97.56%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 78.738%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugin/FormPostContact.php 5 6 83.33%
Totals Coverage Status
Change from base Build 1914974960: 0.09%
Covered Lines: 6025
Relevant Lines: 7652

💛 - Coveralls

Copy link
Contributor

@jayay jayay left a comment

Choose a reason for hiding this comment

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

The migration doesn't look well and should be changed.

plugin/Installer/DatabaseChanges.php Outdated Show resolved Hide resolved
return $this->_pWPDB->query("ALTER TABLE $tableName
ADD COLUMN `contact_type` varchar(255) NULL DEFAULT NULL
");
return "CREATE TABLE $tableName (
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.
The best way to add a column to the table would be to add it to the CREATE TABLE definition in the method getCreateQueryForms.

Then change this block if ($dbversion == 20) { to

if ($dbversion == 20) {
	dbDelta($this->getCreateQueryForms());
	$dbversion = 21;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The new column literally needs to be added in the method getCreateQueryForms.

@hungnc89
Copy link
Contributor Author

@jayay I updated to INSERT IGNORE query

@hungnc89
Copy link
Contributor Author

hungnc89 commented Nov 3, 2021

@jayay sorry. I fixed it

@vu-vuong-esg vu-vuong-esg changed the title 13806 make contact selectable Task#13806 Make contact selectable Nov 4, 2021
@vu-vuong-esg vu-vuong-esg changed the title Task#13806 Make contact selectable P#48327(Task) Make contact selectable Nov 4, 2021
…806-make-contact-selectable

� Conflicts:
�	languages/onoffice-for-wp-websites-de_DE.po
�	plugin/FormPostInterest.php
�	plugin/FormPostOwner.php
�	plugin/Gui/AdminPageFormSettingsContact.php
�	plugin/Installer/DatabaseChanges.php
Copy link
Contributor

@jayay jayay left a comment

Choose a reason for hiding this comment

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

There's still a comment of mine, that's not resolved.

@tung-le-esg
Copy link
Contributor

There's still a comment of mine, that's not resolved.

@hoangdoegs Could you please check the comments and resolve the conflict file?

@tung-le-esg tung-le-esg requested a review from jayay December 9, 2021 08:49
…o 13806-make-contact-selectable

� Conflicts:
�	languages/onoffice-for-wp-websites-de_DE.mo
@michaelarnold-dev
Copy link
Contributor

@hungnc89 Could you please improve the code coverage?

@hoangdoegs
Copy link
Contributor

@hungnc89 Could you please improve the code coverage?

I added more tests. Now the coverage has been improved

@michaelarnold-dev
Copy link
Contributor

michaelarnold-dev commented Jan 26, 2022

Comment of Robert Igelmund Requirement:

"The user can theoretically set the contact type in the scheduled field in "Form Specific Options" (FO) as well as add it to the "Fields" list (FL) so that it is selectable in the frontend form. This may cause a conflict. This case will probably rarely occur in practice. If there is a conflict, the intended field should win in the FO unless no contact type is selected."

The mail to the broker contains in case of conflict the wrong contact type, namely the one selected by the user in the frontend.
The mail should list the contact type that is also written in the address.

That means the error from #2752693 is not fixed with version 2.21.0-27-g8d09228b. See also screenshot.
2022-01-24_17-46-10

@nglelinh
Copy link
Contributor

@jayay please help to confirm that the last feedback from Robert is not the WP issue

@jayay
Copy link
Contributor

jayay commented Jan 31, 2022

@nglelinh The screenshot appears to be correct, but sadly, is in German. It depicts the field kind of contact in the e-mail to be different than the kind of contact of the newly created address, which is the behavior @michaelarnold-dev is referring to here: #88 (comment).

@hoangdoegs
Copy link
Contributor

@jayay Please check again

@michaelarnold-dev michaelarnold-dev merged commit dd6ea27 into master Mar 8, 2022
@jayay jayay deleted the 13806-make-contact-selectable 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.

9 participants