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

Extract selection option labels for Analytics #877

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

Rello
Copy link
Contributor

@Rello Rello commented Feb 27, 2024

  • when column type selection is used, the API needs to deliver the labels, and not the internal ids
    fixes Integration with NC Tables Rello/analytics#385

  • Using lib/Analytics (and not lib/Datasource) for better readability

  • Adopting from LegacyRow to Row

@enjeck
Copy link
Contributor

enjeck commented Apr 16, 2024

@Rello Hi! Are you still working on this?

@Rello Rello requested a review from juliushaertl June 25, 2024 10:47
@Rello
Copy link
Contributor Author

Rello commented Jun 25, 2024

Hi @juliushaertl you think we can merge this?
tests look ok to me if I can judge them correctly?

@blizzz
Copy link
Member

blizzz commented Jun 27, 2024

I am afraid this has potential to be breaking potential existing API use cases. I suppose the IDs of the selection can be fetched already (did not double check). Not that the output is much documented.

There is a conflict btw. If we merge, it's better to have the commits squashed.

@blizzz blizzz added bug Something isn't working 3. to review Waiting for reviews labels Jun 27, 2024
@Rello
Copy link
Contributor Author

Rello commented Jun 27, 2024

@blizzz what do you suggest?
The idea is nice dynamic and the same for tables & forms (potentially for others).
An app can register itself to Analytics.
but all the requirements like tests & co are far more that can be achieved by a hobby project.

I think of skipping this and simply ship the datasources for everything with Analytics itself. Kind of double work because local duplication of the API logic might be required in many places. but not impacting other apps

suggestion?
also applies to
nextcloud/forms#2195

@Rello
Copy link
Contributor Author

Rello commented Jul 3, 2024

as discussed with @blizzz
the Analytics data source was decoupled from the API endpoint to not create breaking changes.
Analytics is taking care of any specific logic on its own

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.

You'll make me extra happy by squashing the commits =)

P.S.: Let me do that quickly

Comment on lines +179 to +180
return ['header' => $header, 'dimensions' => array_slice($header, 0, count($header) - 1), 'data' => $data, //'rawdata' => $data,
'error' => 0,];
Copy link
Member

@blizzz blizzz Jul 3, 2024

Choose a reason for hiding this comment

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

Nitpick-as-a-Service: indentation looks odd. Would be better in the previous style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me check which code styler setting messed this up

Copy link
Member

Choose a reason for hiding this comment

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

saw to late. but a follow up pr is welcomed.

when column type selection is used, the datasource needs the labels, and not the internal ids

Signed-off-by: Rello <Rello@users.noreply.github.com>
@blizzz blizzz mentioned this pull request Jul 3, 2024
@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 3, 2024
@blizzz blizzz merged commit 81342ba into main Jul 3, 2024
62 checks passed
@blizzz blizzz deleted the Rello-analytics branch July 3, 2024 15:19
@blizzz
Copy link
Member

blizzz commented Jul 3, 2024

/backport to stable0.7

@backportbot backportbot bot added the backport-request Pending backport by the backport-bot label Jul 3, 2024
@backportbot backportbot bot removed the backport-request Pending backport by the backport-bot label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration with NC Tables
3 participants