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

41951 Checkbox "Show property status" in unit lists #670

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

dai-eastgate
Copy link
Contributor

related to #642
changed log:
Update the checkbox to show estate status in unit lists

Copy link

github-actions bot commented Nov 7, 2023

Pull Request Test Coverage Report for Build 9541112130

Details

  • 4 of 6 (66.67%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.009%) to 79.593%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plugin/EstateList.php 3 4 75.0%
plugin/EstateUnits.php 0 1 0.0%
Totals Coverage Status
Change from base Build 9461527919: -0.009%
Covered Lines: 8327
Relevant Lines: 10462

💛 - Coveralls

@fredericalpers fredericalpers linked an issue Nov 8, 2023 that may be closed by this pull request
@fredericalpers fredericalpers added this to the v4.18 milestone Nov 8, 2023
@fredericalpers fredericalpers added the QA Issue or Pull request that is in review label Nov 8, 2023
@fredericalpers fredericalpers modified the milestones: v4.18, v4.19 Feb 12, 2024
@fredericalpers fredericalpers added UI/UX Issue, Pull Request or Discussion related to UI/UX component: unit list Issue, Pull Request or Discussion related to unit lists labels Feb 20, 2024
Copy link
Contributor

@andernath andernath left a comment

Choose a reason for hiding this comment

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

@dai-eastgate I have a question. Please take a look.

@@ -234,4 +235,20 @@ public function getInputModelCustomLabelLanguageSwitch(): InputModelDB
});
return $pInputModel;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This function already exist in FormModelBuilderDBEstateListSettings
Why would you duplicate it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion. The function was indeed duplicated, and I have updated the code to remove it. Could you please review it again? Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem @dai-eastgate :)
Could you first take a look at the unit tests please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andernath The unit tests have been addressed in the PR at oo-wp-plugin#792. Could you please review it first?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dai-eastgate Ops sorry I my fault, missed your comment from yesterday. Of course I will start reviewing fist. :)

Copy link

github-actions bot commented Apr 4, 2024

Steps to install the approved version:

  1. Download onoffice-4.17-9-g15e942f0-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8551428475.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

andernath
andernath previously approved these changes Apr 4, 2024
Copy link

github-actions bot commented Apr 4, 2024

Steps to install the approved version:

  1. Download onoffice-4.18.1-18-g230d5b9d-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8552401526.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@fredericalpers
Copy link
Contributor

fredericalpers commented Apr 8, 2024

@dai-eastgate While testing I noticed that the implementation of this feature only applies to the unit lists inside of an master property. As seen in the screenshot below. However, the status is also inherited from the "normal" detail view to the unit detail view. The checkbox must therefore also affect the status banner inherited from the detail view as seen in the second screenshot below.

How to setup the unit lists: https://wp-plugin.onoffice.com/en/advanced-features/unit-lists/

Marketing status unit list

vermarktungsstatus

Marketing status detail view

vermarktungsstatus2

@dai-eastgate
Copy link
Contributor Author

@fredericalpers I will check it

@dai-eastgate
Copy link
Contributor Author

@fredericalpers @andernath I fixed it and during testing, I discovered an issue where the unit detail was not loading the necessary script. I've fixed it in this PR, could you take a look?
Here's a video demo: https://files.fm/u/xd5qm6bezg#/view/4gxenfcyxk

@dai-eastgate dai-eastgate force-pushed the 41951-checkbox-show-property-status-in-unit-lists branch from 8e6364f to 02bf0fc Compare April 11, 2024 03:20
@dai-eastgate dai-eastgate force-pushed the 41951-checkbox-show-property-status-in-unit-lists branch from 02bf0fc to 9982761 Compare April 11, 2024 03:21
andernath
andernath previously approved these changes Apr 15, 2024
Copy link

Steps to install the approved version:

  1. Download onoffice-4.18.1-27-gd1783fcf-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8683595283.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@fredericalpers
Copy link
Contributor

@dai-eastgate unfortunately I think I did not make it clear enough. As the property status gets inherited from the "normal" detail page, even tho I disabled it for unit detail pages, it will still show.

Show property status disabled for unit detail pages = no property status should be displayed on unit detail pages at all. Currently still the inherited one from "normal" detail pages will be displayed.

@fredericalpers fredericalpers modified the milestones: v4.19, v4.20 Apr 15, 2024
@dai-eastgate
Copy link
Contributor Author

@dai-eastgate unfortunately I think I did not make it clear enough. As the property status gets inherited from the "normal" detail page, even tho I disabled it for unit detail pages, it will still show.

Show property status disabled for unit detail pages = no property status should be displayed on unit detail pages at all. Currently still the inherited one from "normal" detail pages will be displayed.

@fredericalpers @andernath I have fixed it and here is video demo, please review and test this branch again. Thanks!

https://files.fm/u/ey5z2d86xd#/view/xhb9zy26bf

Copy link

Steps to install the approved version:

  1. Download onoffice-4.19-13-g5829d8e8-please-unpack.zip from https://github.com/onOffice-Web-Org/oo-wp-plugin/actions/runs/8795705156.
  2. Unpack the downloaded file to get another .zip file.
  3. Upload that inner .zip file to WordPress.

@andernath
Copy link
Contributor

@fredericalpers ready for testing

@fredericalpers fredericalpers removed this from the v4.20 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: unit list Issue, Pull Request or Discussion related to unit lists QA Issue or Pull request that is in review UI/UX Issue, Pull Request or Discussion related to UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox "Show property status" in unit lists
3 participants