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

feat: add backend for new user/group column #1090

Merged
merged 20 commits into from
Jul 31, 2024
Merged

Conversation

enjeck
Copy link
Contributor

@enjeck enjeck commented May 13, 2024

Contributes to #586

This adds to the backend to support adding a new column that receives users or groups.

We expect the row values of this column to be array of objects with format {id: string, type: int}

Creating a column

Request

curl --silent \
    -X POST \
    -u admin:admin \
    -H "OCS-APIRequest: true" \
    -H "Content-Type: application/json" \
    -d '{ "baseNodeId": 1, "title": "New Column", "usergroupDefault": "[{\"id\": \"admin\", \"type\": 0}]" }' \
'http://nextcloud.local/ocs/v2.php/apps/tables/api/2/columns/usergroup?format=json' | jq

Result

{
  "ocs": {
    "meta": {
      "status": "ok",
      "statuscode": 200,
      "message": "OK"
    },
    "data": {
      "id": 7,
      "tableId": 1,
      "title": "New Column",
      "createdBy": "admin",
      "createdByDisplayName": "admin",
      "createdAt": "2024-05-13 15:25:29",
      "lastEditBy": "admin",
      "lastEditByDisplayName": "admin",
      "lastEditAt": "2024-05-13 15:25:29",
      "type": "usergroup",
      "subtype": "",
      "mandatory": false,
      "description": "",
      "numberDefault": null,
      "numberMin": null,
      "numberMax": null,
      "numberDecimals": null,
      "numberPrefix": "",
      "numberSuffix": "",
      "textDefault": null,
      "textAllowedPattern": null,
      "textMaxLength": null,
      "selectionOptions": [],
      "selectionDefault": null,
      "datetimeDefault": null,
      "usergroupDefault": [
        {
          "id": "admin",
          "type": 0,
        }
      ],
      "usergroupMultipleItems": null,
      "usergroupSelectUsers": null,
      "usergroupSelectGroups": null,
      "showUserStatus": null
    }
  }
}

This comment was marked as off-topic.

@enjeck enjeck changed the base branch from main to consolidate-ncselect June 4, 2024 08:31
@enjeck enjeck self-assigned this Jun 4, 2024
@enjeck enjeck requested a review from juliusknorr June 4, 2024 09:33
@juliusknorr juliusknorr linked an issue Jun 6, 2024 that may be closed by this pull request
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

First glance looks good, have not tested myself yet

  • Integration test can be extended around inserting, modifying and delete column values
  • a new migration class is needed to add the new columns on existing installations (existing steps are not executed again).

lib/Db/Column.php Outdated Show resolved Hide resolved
lib/Db/Column.php Outdated Show resolved Hide resolved
lib/Db/LegacyRowMapper.php Outdated Show resolved Hide resolved
lib/Db/Row2Mapper.php Outdated Show resolved Hide resolved
lib/Db/Row2Mapper.php Show resolved Hide resolved
lib/Db/Row2Mapper.php Outdated Show resolved Hide resolved
lib/Db/Row2Mapper.php Outdated Show resolved Hide resolved
lib/Db/Row2Mapper.php Outdated Show resolved Hide resolved
lib/Service/ColumnTypes/UsergroupBusiness.php Outdated Show resolved Hide resolved
lib/Db/ColumnTypes/UsergroupColumnQB.php Show resolved Hide resolved
@enjeck enjeck force-pushed the consolidate-ncselect branch 2 times, most recently from ef7e9f2 to 87e80be Compare June 20, 2024 06:59
Base automatically changed from consolidate-ncselect to main June 27, 2024 10:26
@enjeck enjeck marked this pull request as ready for review July 2, 2024 21:24
@enjeck enjeck force-pushed the new-usergroup-column branch 3 times, most recently from d07cbe5 to 5c276de Compare July 8, 2024 04:41
@juliusknorr
Copy link
Member

All right, from the journey through the rabbit hole of value transformation/parsing, i came up with a few changes:

Related to this PR:

  • The value_type column is now only added to the usergroup cell data table
  • I cleaned up the migrations a bit further
  • I added the to filter by one value in views for the user group column

More general as the current approach to handle different value formats was really confusing and not feasible for the usergroup column:

  • Improve mapper methods to be more clear on what they do
    • parseValueIncoming: This had two use cases, one to map from the request data to the value to insert cell data into the database, but also to map from filter values to the value that is used in the query builder to construct view filter queries. It was split into
      • filterValueToQueryParam: For a stored value of a filter rule get the value to put in the database query, this could in the future be refactored again to actually build queries more dynamically, but left that out to keep the changes at a reasonable level
      • For the case of inserting/updating values we no longer pass encode/decode json values, but an applyDataToEntity method is added for each case to just fill the Entity object depending on the type.
    • parseValueOutgoing
      • Changed to a formatEntity and no longer use a mixed value type but the actual database entity so we know what to expect and how to transform that to the actual response data format
    • Use hasMultipleValues to indicate that multiple values are stored in multiple entities compared to a hardcoded column type
  • Removed a few duplicate code bits around getting the mapper class dynamically

Still checking in on the test failures

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

👍 But would appreciate @blizzz and @enjeck to also give their thumbs up for my parts

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

A question, a hint, a follow up thing, and a nitpick, nothing blocking per se.

lib/Capabilities.php Show resolved Hide resolved
lib/Controller/Api1Controller.php Outdated Show resolved Hide resolved
->from('tables_row_cells_'.$columnType)
->selectAlias($qb->expr()->castColumn('value', IQueryBuilder::PARAM_STR), 'value');

// This is not ideal but I cannot think of a good way to abstract this away into the mapper right now
Copy link
Member

Choose a reason for hiding this comment

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

roger. perhaps opening an issue for this afterwards?

lib/Migration/Version000800Date20240712000000.php Outdated Show resolved Hide resolved
@juliusknorr juliusknorr merged commit a480ad7 into main Jul 31, 2024
59 checks passed
@juliusknorr juliusknorr deleted the new-usergroup-column branch July 31, 2024 18:01
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.

Column type user and groups
4 participants