-
Notifications
You must be signed in to change notification settings - Fork 80
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
toc_generation service #3465
Merged
Merged
toc_generation service #3465
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
8ad80a4
toc_generation service, WIP
daneryl 282a5a0
aggregations for generatedToc property
daneryl 60263ad
review toc endpoint
daneryl 77b0532
tocService as a feature and cronjob
daneryl 2883177
customFilters for search endpoint
daneryl bcdb188
UI toc generation components, WIP
daneryl baa08e5
added basic styles on autogenerated toc
42e7dfd
toc_generation service, WIP
daneryl 97405a3
aggregations for generatedToc property
daneryl ed1565a
review toc endpoint
daneryl 2023a22
tocService as a feature and cronjob
daneryl 5f88a16
customFilters for search endpoint
daneryl 08bea1f
UI toc generation components, WIP
daneryl d1cb5d3
added basic styles on autogenerated toc
305f49a
Merge branch '3447_toc_service_integration' of https://github.com/hur…
5650300
Updated autogenerated toc
grafitto 8868f57
fix lint errors
daneryl f632ead
fix blank state panel
daneryl eb09f2f
fix e2e
daneryl eba2caf
prevent crash on toc service unavailable
daneryl 5928bb0
Removed unnecessary scss import
d8ef75b
fixed/refactor styles
daneryl adb875a
Rename spec describe
daneryl e0cff38
removed comment
daneryl cc42af9
fixed emit types
daneryl 4967e1a
ignored eslint on spec
daneryl 1cde69b
review fixes
daneryl d87b298
Removed for loop on toc scss
aa2f46b
uploadFile returns the full response
daneryl e1556b8
error handling on tocService
daneryl a32f998
icon for review toc
daneryl 97efec7
filteredAggregations labels now using t function
daneryl 0a6cd50
Merge branch 'development' into 3447_toc_service_integration
konzz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are already fetching the entities, should it be worth it to check if this process would actually change the value before producing the save on line 42?
I mean, do we risk it to just save even if there would be no changes or do we make the reduce on line 47 here and only save if there are actual changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do not see any benefit other than a really small performance impact ?, adding a condition will add complexity. do you foresee any problems saving entities with the same values ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that conditionals should happen at some point. So, if not here, then the entities save, when updating all the connected entities, should know: hey, nothing changed. But if this save actually triggers an full entity check and reindex on all connected entities, I would rather have the conditional here. If you are confident that is not going to happen, I agree a single save (or potentially n entities depending on languages) is not a huge deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RafaPolit im not confident its not going to happen, but if saving one entity can be this problematic i believe this should be handled at a lower level, not anytime we need to save one ? tech debt for this pls ?