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: enable user defined 'id' for reports so that the get URL can be stable. Breaking change as 'id' will be required to function. #4520

Merged
merged 19 commits into from
Dec 3, 2024

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Nov 26, 2024

What are the main changes you did

  • enable user defined ID for reports because now URLs were not stable (report index could change depending on load order)

How to test

  • view the report and see that the URL now uses the id if exists

Question to reviewers:

  • can we make this a breaking change by stopping use of the index as alternative id?
  • do you want to remove 'name' or should we use it as human readable 'label' in report list?
  • we could change URL to be of pattern /report/id or in case of multiple /report/id,id2 to make it more look like REST

enhancements skipped in view of time

  • uniqueness check
  • regexp to make id be a-zA-Z0-9 (certainly shouldn't contain ',')

@mswertz mswertz marked this pull request as draft November 26, 2024 21:53
@mswertz mswertz marked this pull request as ready for review November 27, 2024 19:19
@mswertz mswertz changed the title feat: enable user defined 'id' for reports so that the get URL can be stable !feat: enable user defined 'id' for reports so that the get URL can be stable. Breaking change as 'id' will be required to function. Nov 28, 2024
@mswertz mswertz requested a review from harmbrugge November 28, 2024 18:54
Copy link
Member

@harmbrugge harmbrugge left a comment

Choose a reason for hiding this comment

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

Code looks good

Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

This is a really good feature that will help us with the dashboards and other custom applications. I have a few comments about the documentation as well as some potential limitations.

Questions / pitfalls

Should there be strict rules regarding the ID of the report? For example, I created a report in the preview called "country summary". Calling the report over the api at https://preview-emx2-pr-4520.dev.molgenis.org/test/api/report/country%20summary/ returns a "file not found" error. I would expect it to work as noted in the documentation. If I rename the report to "country", then it works fine.

What is the correct API URL for the reports?

Are there multiple options for requesting a report? This is a follow up to the previous item. There are multiple endpoints for retrieving a report output. Is this expected?

http://<host>/<schema>/api/reports/<report>
http://<host>/<schema>/api/reports/json?id=<report>

docs/molgenis/use_reports.md Outdated Show resolved Hide resolved
@mswertz mswertz requested a review from davidruvolo51 December 2, 2024 14:17
Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

Looks good! I think we can merge.

@mswertz mswertz merged commit 77fa01e into master Dec 3, 2024
7 checks passed
@mswertz mswertz deleted the feat/report_editable_id branch December 3, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants