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

Support all entity types in reports #1711

Merged
merged 11 commits into from
Feb 16, 2023
Merged

Support all entity types in reports #1711

merged 11 commits into from
Feb 16, 2023

Conversation

TheSlimvReal
Copy link
Collaborator

see issue: #1682

Visible/Frontend Changes

--

Architectural/Backend Changes

  • query string is checked for referenced entities which are then loaded individually

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2023

Deployed to https://pr-1711.aam-digital.net/

Copy link
Member

@sleidig sleidig 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 and basic reports tested successfully for a custom extended entity type

feel free to have a look at PR #1216 also and close, amend or finish that one, if it seems feasible after having picked up this topic in general 😊

@TheSlimvReal TheSlimvReal requested a review from sleidig February 9, 2023 17:52
@TheSlimvReal
Copy link
Collaborator Author

@sleidig updates of the cache are now also implemented. Please have a look at the new changes

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

please have a last look at those subscriptions and potential memory implications. Otherwise reports seem to work and code looks good 👍

this.entityInfo[ENTITY_TYPE].updating = true;
this.entityMapper
.receiveUpdates(ENTITY_TYPE)
.subscribe(({ entity, type }) => {
Copy link
Member

Choose a reason for hiding this comment

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

will this not re-subscribe to the same entity type updates after a broader date range of that type is loaded?

And in general, when are we cleaning up these subscriptions? (or if we don't yet, shouldn't we clear that cache at some time rather than keeping all that stuff in memory forever after a user once used a report?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the updating flag is only set to true once and after that this entity type will always be filtered out here.

Generally, there is no concept of cleaning up yet as this service lives outside of the component.
A option would be to just clear the cache once the user leaves the site but then this whole caching concept was pointless (as you can make changes without going away from the reporting page).

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@TheSlimvReal TheSlimvReal merged commit 2c37faf into master Feb 16, 2023
@TheSlimvReal TheSlimvReal deleted the report_all_entities branch February 16, 2023 09:48
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.18.1-master.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.18.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants