Skip to content

Conversation

@AIlkiv
Copy link
Contributor

@AIlkiv AIlkiv commented Jul 8, 2025

  • Moved the logic of value parsing and display value parsing to separate methods (parseValue / parseDisplayValue and canBeParsed / canBeParsedDisplayValue) for the SelectionBusiness type.
  • Updated calls in ImportService to use the new methods for display value.
  • Added default implementations in SuperBusiness to maintain backward compatibility.
  • Improved readability and separation of responsibilities in classes, which simplifies future support and extensions.

These changes allow for a clear separation of ID parsing and display value parsing, which is important for correct import and validation of data in tables.

@AIlkiv AIlkiv requested review from blizzz and enjeck as code owners July 8, 2025 12:43
@AIlkiv AIlkiv force-pushed the chore-split-parse-methods branch from d9ae0b9 to 92ad414 Compare July 8, 2025 12:50
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

Do we need to update

if ($columnBusiness->canBeParsed($value, $column)) {
return json_decode($columnBusiness->parseValue($value, $column), true);
}
too to use the new functions?

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Jul 18, 2025

Do we need to update

if ($columnBusiness->canBeParsed($value, $column)) {
return json_decode($columnBusiness->parseValue($value, $column), true);
}

too to use the new functions?

@enjeck No, because in this place for type selection there is already a value id

@github-actions
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

it would be nice to also have a test for the selection column, considering we have https://github.com/nextcloud/tables/blob/main/tests/unit/Service/ColumnTypes/TextLinkBusinessTest.php too

@enjeck
Copy link
Contributor

enjeck commented Aug 12, 2025

it would be nice to also have a test for the selection column, considering we have https://github.com/nextcloud/tables/blob/main/tests/unit/Service/ColumnTypes/TextLinkBusinessTest.php too

Created new issue at #1994

@enjeck enjeck force-pushed the chore-split-parse-methods branch from 92ad414 to 49ff88b Compare August 12, 2025 02:35
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

I'm happy with how this looks, will wait for @blizzz to check too

@AIlkiv AIlkiv force-pushed the chore-split-parse-methods branch 2 times, most recently from a02d387 to b05eb15 Compare August 16, 2025 10:28
@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 16, 2025

@enjeck
I added tests

Signed-off-by: ailkiv <a.ilkiv.ye@gmail.com>
@enjeck enjeck merged commit b566f02 into nextcloud:main Sep 1, 2025
62 of 64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants