-
Notifications
You must be signed in to change notification settings - Fork 336
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
[ENG-6125] Implement Required Changes to Institution-Metric Summary API #10757
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
from osf.metrics import InstitutionProjectCounts | ||
from osf.models import OSFUser, Node, Institution, Registration | ||
from osf.metrics import UserInstitutionProjectCounts | ||
from osf.metrics.reports import InstitutionalUserReport | ||
from osf.metrics.reports import InstitutionalUserReport, InstitutionMonthlySummaryReport | ||
from osf.utils import permissions as osf_permissions | ||
|
||
from api.base import permissions as base_permissions | ||
|
@@ -48,6 +48,7 @@ | |
InstitutionDepartmentMetricsSerializer, | ||
NewInstitutionUserMetricsSerializer, | ||
OldInstitutionUserMetricsSerializer, | ||
NewInstitutionSummaryMetricsSerializer, | ||
) | ||
from api.institutions.permissions import UserIsAffiliated | ||
from api.institutions.renderers import InstitutionDepartmentMetricsCSVRenderer, InstitutionUserMetricsCSVRenderer, MetricsCSVRenderer | ||
|
@@ -391,7 +392,7 @@ def create(self, *args, **kwargs): | |
return ret | ||
|
||
|
||
class InstitutionSummaryMetrics(JSONAPIBaseView, generics.RetrieveAPIView, InstitutionMixin): | ||
class _OldInstitutionSummaryMetrics(JSONAPIBaseView, generics.RetrieveAPIView, InstitutionMixin): | ||
permission_classes = ( | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
|
@@ -581,6 +582,53 @@ def get_default_search(self): | |
) | ||
|
||
|
||
class _NewInstitutionSummaryMetricsList(JSONAPIBaseView, generics.RetrieveAPIView, InstitutionMixin): | ||
'''list view for institution-summary metrics | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. either keep it an actual list view (which should be easy with this sort of inconsistency is actively (tho subtly) hostile to future contributors |
||
used only when the INSTITUTIONAL_DASHBOARD_2024 feature flag is active | ||
(and should be renamed without "New" when that flag is permanently active) | ||
''' | ||
permission_classes = ( | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
base_permissions.TokenHasScope, | ||
IsInstitutionalMetricsUser, | ||
) | ||
|
||
required_read_scopes = [CoreScopes.INSTITUTION_METRICS_READ] | ||
required_write_scopes = [CoreScopes.NULL] | ||
|
||
view_category = 'institutions' | ||
view_name = 'institution-summary-metrics' | ||
|
||
serializer_class = NewInstitutionSummaryMetricsSerializer | ||
|
||
def get_object(self): | ||
institution = self.get_institution() | ||
search_object = self.get_default_search() | ||
if search_object: | ||
object = search_object.execute()[0] | ||
object.id = institution._id | ||
return object | ||
|
||
def get_default_search(self): | ||
_yearmonth = InstitutionMonthlySummaryReport.most_recent_yearmonth() | ||
if _yearmonth is None: | ||
return None | ||
return ( | ||
InstitutionMonthlySummaryReport.search() | ||
.filter('term', report_yearmonth=str(_yearmonth)) | ||
.filter('term', institution_id=self.get_institution()._id) | ||
) | ||
|
||
|
||
institution_summary_metrics_list_view = toggle_view_by_flag( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you make it a list view, this is fine, but if it's a detail view (as implemented now) it shouldn't be called |
||
flag_name=osf.features.INSTITUTIONAL_DASHBOARD_2024, | ||
old_view=_OldInstitutionSummaryMetrics.as_view(), | ||
new_view=_NewInstitutionSummaryMetricsList.as_view(), | ||
) | ||
institution_summary_metrics_list_view.view_name = 'institution-summary-metrics' | ||
|
||
|
||
institution_user_metrics_list_view = toggle_view_by_flag( | ||
flag_name=osf.features.INSTITUTIONAL_DASHBOARD_2024, | ||
old_view=_OldInstitutionUserMetricsList.as_view(), | ||
|
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.
nit: i'd say this kind of info is not helpful in the docstring, since it duplicates info in the fields themselves and is unlikely to be kept in sync as fields are added/removed over time (again, thinking of confusion for future contributors) -- and here it's already out of sync (missing the last four fields)