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

add column types for IPv4 and IPv6 addresses, compatible with both text and numeric filters, thanks to natural sort #1097

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

kirisakow
Copy link
Contributor

@kirisakow kirisakow commented May 19, 2024

This is an attempt to respond to issue #402 (and #981) by implementing TextIPv4Address and TextIPv6Address column types that are compatible with both text and numeric FilterIds (all nine of them), thanks to natural sort.

This is my first PR on this project, so please feel free to kindly point out anything I may have missed or done wrong.

@kirisakow kirisakow marked this pull request as draft May 19, 2024 16:06
@kirisakow kirisakow marked this pull request as ready for review May 20, 2024 12:31
@enjeck
Copy link
Contributor

enjeck commented May 20, 2024

@kirisakow Thanks for working on this. Coincidentally, I've also been working on a new column type (for users/groups) at: #1090. Maybe the files and feedback I'm getting over there helps 😄

@enjeck
Copy link
Contributor

enjeck commented May 20, 2024

@kirisakow While working on #1090, I wrote down some notes of files I potentially had to change/add. I think not everything is applicable here, but might be helpful:

Frontend

  • In src/shared/components/ncTable/mixins/columnsTypes, we create a new column type that specifies:
    • properties the column has
    • converting value to string representation (and using for search and filter operations)
    • sort function
  • In src/shared/components/ncTable/mixins/magicFields.js, we specify if the new column fits any particular magicfield
  • In src/shared/components/ncTable/mixins/filters.js, we specify if the new column should be shown for any particular filter
  • In src/shared/components/ncTable/mixins/columnParser.js, we specify that this new column is valid
  • In src/shared/components/ncTable/mixins/columnHandler.js, add the column and subtypes if any
  • In src/modules/modals/EditColumn.vue and src/modules/modals/CreateColumn.vue, specify how to handle column creation and editing
  • in src/modules/main/partials/ColumnFormComponent.vue, add the column
  • In src/modules/main/partials/ColumnTypeSelection.vue, add icon and text that will appear when user initially chooses a column type
  • In src/shared/constants.js , specify new column type
  • In src/shared/components/ncTable/mixins/columnClass.js, maybe add Abstract column
  • In src/shared/components/ncTable/mixins/columnHandler.js, add datatype
  • In src/shared/components/ncTable/mixins/columnParser.js, add type
  • In src/shared/components/ncTable/mixins/metaColumns.js, maybe add meta type
  • In src/shared/components/ncTable, add new TableCell component file for rendering the type in the table.
  • In src/shared/components/ncTable/partials/columnTypePartials/forms, add a new Form component that will be inserted into the create/edit form when we try to create a new column for this new type. Will have to create this for subtypes too
  • In src/shared/components/ncTable/partials/rowTypePartials, add a new Form component that will be inserted into the create/edit form when we try to create a new row for this new type. Will have to create this for subtypes too, if there are any

Backend

  • In lib/Service/ColumnService.php, specify properties
  • In lib/Service/ColumnTypes, add a new column type
  • In `lib/Db/Column.php, add new column type
  • Add column properties to lib/Service/TableTemplateService.php
  • Add to lib/Controller/ColumnController.php the properties for new type
  • Add type to lib/Controller/ApiColumnsController.php
  • Add column details to lib/ResponseDefinitions.php
  • Add column type to lib/Capabilities.php
  • Add route to create column to appinfo/routes.php
  • Add properties to lib/Controller/Api1Controller.php
  • Add new column type file to lib/Db/ColumnTypes
  • Add to new column data lib/Db/LegacyRowMapper.php
  • Add to lib/Db/Row2Mapper.php
  • Add new RowCell and RowCellMapper php file for the new column type to lib/Db
  • Add column type to lib/Helper/ColumnsHelper.php
  • Update migration files to include new type. Look at all files at lib/Migration most especially lib/Migration/Version000000Date20210921000000.php and lib/Migration/Version000700Date20230916000000.php
  • Maybe add to lib/Service/ImportService.php

Others

  • Ensure types are added to src/types/openapi/openapi.ts
  • Add bdd test to tests/integration/features/APIv2.feature
  • Ensure types are added to openapi.json
  • Write e2e tests

@kirisakow
Copy link
Contributor Author

kirisakow commented May 21, 2024

@enjeck Thank you for the input, that was of much help!

I assume development is now finished for the

  • front-end part;
  • back-end part;

Still, a few TODOs remain — like things I wasn't sure about:

  • I'd appreciate for the core logic code to be reviewed by someone possessing more experience than me:
    • src/shared/components/ncTable/mixins/columnsTypes/textIPv4Address.js
    • src/shared/components/ncTable/mixins/columnsTypes/textIPv6Address.js
  • specify valid IPv4 and IPv6 regex patterns, as I wasn't sure where and how to declare a value of textAllowedPattern;
  • every thing tests-related — unit tests, integration tests;
  • every thing OpenAPI-related*;
  • every thing migrations-related*;

* probably nothing to do about OpenAPI or migrations, as this feature merely adds two subtypes to the existing text type.

- change subtype value declared in `columnHandler.js` and its occurrencies in `CreateColumn.vue`;
- rename the corresponding .vue files;
- refactor the corresponding imports.
@kirisakow kirisakow marked this pull request as draft May 22, 2024 20:24
Copy link
Contributor

github-actions bot commented Jun 3, 2024

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!

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