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

getDocument returns relations. #3572

Merged
merged 1 commit into from
Apr 5, 2021
Merged

Conversation

daneryl
Copy link
Collaborator

@daneryl daneryl commented Mar 30, 2021

fixes #3499

PR checklist:

  • Update READ.me ?
  • Update API documentation ?

QA checklist:

  • Smoke test the functionality described in the issue
  • Test for side effects
  • UI responsiveness
  • Cross browser testing
  • Code review

Sorry, something went wrong.

@daneryl daneryl force-pushed the 3499_error_ref_scroll branch 2 times, most recently from 213afc6 to 332139d Compare March 30, 2021 12:12
@daneryl daneryl requested a review from RafaPolit March 30, 2021 12:22
@daneryl daneryl marked this pull request as ready for review March 30, 2021 12:27
Copy link
Member

@RafaPolit RafaPolit left a comment

Choose a reason for hiding this comment

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

I am wondering if polluting 6 files with .delete('relations').toJS() is the right approach. I understand it is the solution, but if we later on want to actually use them, or find better solutions, finding this spread out within the code is not great.

Perhaps we can abstract that in a utility like sanitizeDoc somewhere that concentrates this delete in a single location?

Also, I'm not sure that the removed omitRelationships in app/react/Viewer/actions/documentActions.js is without impact for queries like a countries that could have thousands of relationships.

@daneryl daneryl force-pushed the 3499_error_ref_scroll branch 4 times, most recently from 6b434cd to 238249b Compare April 1, 2021 08:17
@daneryl
Copy link
Collaborator Author

daneryl commented Apr 1, 2021

I am wondering if polluting 6 files with .delete('relations').toJS() is the right approach. I understand it is the solution, but if we later on want to actually use them, or find better solutions, finding this spread out within the code is not great.

Perhaps we can abstract that in a utility like sanitizeDoc somewhere that concentrates this delete in a single location?

done 👍

Also, I'm not sure that the removed omitRelationships in app/react/Viewer/actions/documentActions.js is without impact for queries like a countries that could have thousands of relationships.

i thought that the relationships returned were hard coded to 300 limit, this is not true, and only happens when asking for text relationships, hard coding a limit here can have unexpected ramifications.
I reverted back the omitRelatonships by default and instead opted for assigning on the requestState the same textReferences requested to the doc.relations, this way there is an exact match including not exceeding the 300 limit, let me know what you think @RafaPolit

index a3ca9a9fe..ae3cc7c89 100644
--- a/app/react/Viewer/actions/routeActions.js
+++ b/app/react/Viewer/actions/routeActions.js
@@ -47,7 +47,10 @@ export async function requestViewerState(requestParams, globalResources) {
   return [
     setViewerState({
       documentViewer: {
-        doc,
+        doc: {
+          ...doc,
+          relations: references,
+        },
         references,
         relationTypes,
         rawText,

@RafaPolit RafaPolit merged commit ac7220d into development Apr 5, 2021
@RafaPolit RafaPolit deleted the 3499_error_ref_scroll branch April 5, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants