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

serializers: inject ui config in aggregation buckets #213

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

zzacharo
Copy link
Member

No description provided.

@zzacharo zzacharo added this to In Review in InvenioRDM October Board Oct 20, 2020
Comment on lines +60 to +62
if bucket.get("subtype"):
bucket["subtype"]["buckets"] = list(map(
partial(self._set_bucket_label, labels_map),
bucket["subtype"]["buckets"]))
Copy link
Member

Choose a reason for hiding this comment

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

minor: This logic is repeated a few times. Can it be made a helper function?

@Glignos Glignos self-requested a review October 20, 2020 11:54
return dict(
access_right=dict(
category=obj["access"]["access_right"]
))

def _serialize_ui_options_from_vocabulary(
Copy link
Member

Choose a reason for hiding this comment

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

This function works for the currently present Vocabularies but maybe it's worth considering a recursive approach if we expect depth>2 in any future ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

ping @lnielsen

bucket_label = labels_map.get(bucket["key"])
if bucket_label:
bucket["label"] = bucket_label
if bucket.get("subtype"):
Copy link
Member

Choose a reason for hiding this comment

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

Moderate: subtype is a specific key for this vocabulary, I think we need a more general approach. I think we can safely assume in this case that any extra key corresponds to a sub-bucket key, but we could also check the existence of the buckets key in the nested object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked Zenodo and it seems we use the subtype key for child aggregations. Of course, we should force this in our UI configuration. I assumed that we can normalize the interface by having all nested aggregations under subtype field. If not, then it needs some more dynamic checking but not sure if it's worth it... @lnielsen WDYT?

"""
bucket_label = labels_map.get(bucket["key"])
if bucket_label:
bucket["label"] = bucket_label
Copy link
Member

Choose a reason for hiding this comment

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

Minor: We should add the doc_count to the label here to keep the same visuals.

@zzacharo zzacharo force-pushed the override-agg-labels branch 2 times, most recently from ecb2c75 to d40eeb4 Compare October 20, 2020 12:10
partial(self._set_bucket_label, label_map),
buckets))

def _serialize_resource_type_agg(self, resource_type_agg):
Copy link
Member

Choose a reason for hiding this comment

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

Minor: These functions could probably be merged in a generic one using only a key and the top bucket.

@zzacharo zzacharo force-pushed the override-agg-labels branch 4 times, most recently from 48cf275 to c169e48 Compare October 20, 2020 14:01
Copy link
Member

@Glignos Glignos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 86.095% when pulling 166b584 on zzacharo:override-agg-labels into 575d586 on inveniosoftware:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 86.095% when pulling 166b584 on zzacharo:override-agg-labels into 575d586 on inveniosoftware:master.

@zzacharo zzacharo merged commit 664d45b into inveniosoftware:master Oct 20, 2020
InvenioRDM October Board automation moved this from In Review to Done Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants