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

Adding last updated to summary endpoint [ENG-1879] #9418

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

corbinSanders
Copy link
Contributor

Purpose

The front end needs the datetime of when institutional metrics were last updated

Changes

Adding field to serializer. Getting the last updated datetime. Updating tests

QA Notes

  • Does this change require a data migration? If so, what data will we migrate?
    No
  • What is the level of risk?
    minimal
    • Any permissions code touched?
      No
    • Is this an additive or subtractive change, other?
      Additive
  • How can QA verify? (Through UI, API, AdminApp or AdminAdminApp?)
    API
    • If verifying through API, what's the new version? Please include the endpoints in PR notes or Dev docs.
      No new version. Use v2/institutions/<institution_id>/metrics/summary/
  • What features or workflows might this change impact?
    Institutional dashboard
  • How will this impact performance?
    Shouldn't
    -->

Documentation

N/A

Side Effects

N/A

Ticket

https://openscience.atlassian.net/browse/ENG-1879

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Very nice.

Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

hm wait, a test failure -- unsure if related?

Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

looks like test failure is probably unrelated to this, but'll still need to run successfully before merge

@corbinSanders corbinSanders force-pushed the feature/last_updated_summary_metrics branch from f097420 to 2d1ab79 Compare June 17, 2020 15:54
@corbinSanders corbinSanders force-pushed the feature/last_updated_summary_metrics branch from 2d1ab79 to a76eae0 Compare June 17, 2020 16:21
@@ -402,7 +402,9 @@ class InstitutionSummaryMetrics(JSONAPIBaseView, generics.RetrieveAPIView, Insti
# overrides RetrieveAPIView
def get_object(self):
institution = self.get_institution()
return InstitutionProjectCounts.get_latest_institution_project_document(institution)
results = InstitutionProjectCounts.get_latest_institution_project_document(institution)
results['last_updated'] = UserInstitutionProjectCounts.get_recent_datetime(institution)
Copy link
Contributor

Choose a reason for hiding this comment

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

@corbinSanders thinking on this again, it looks like all metrics should have a timestamp field -- is it possible to get the timestamp off the "latest" InstitutionProjectCounts result that's already being fetched? rather than make additional requests to elastic for UserInstitutionProjectCounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌

@corbinSanders corbinSanders force-pushed the feature/last_updated_summary_metrics branch from 889ec1c to 7584a40 Compare June 18, 2020 15:18
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.

None yet

3 participants