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

[ENG-6077] add link_to_external_reports_archive to institutions #10708

Merged

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Aug 19, 2024

Purpose

allow each institution to have a link to an archive of past quarterly metrics reports

Changes

  • add link_to_external_reports_archive field to the Institution model
  • add reusable ShowIfObjectPermission serializer field wrapper
  • show link_to_external_reports_archive on institutions in the api, but only for users with "view_institutional_metrics" object permission
  • (for consistency) use ShowIfObjectPermission to hide existing institutional metrics relationship fields for users that do not have permission to see them

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

ENG-6077

Copy link
Member

@mfraezz mfraezz left a comment

Choose a reason for hiding this comment

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

One general question, otherwise LGTM :octocat:

  1. All of the related metric views also check the view_institutional_metrics perm, so hiding the fields shouldn't be a major concern. However, since it's a possible that removing them could affect API consumers, should that be a versioned change, or is the risk sufficiently small?

@aaxelb
Copy link
Contributor Author

aaxelb commented Aug 19, 2024

@mfraezz

All of the related metric views also check the view_institutional_metrics perm, so hiding the fields shouldn't be a major concern. However, since it's a possible that removing them could affect API consumers, should that be a versioned change, or is the risk sufficiently small?

hmm good point... it is possible the missing relationships could break client code, but i'd consider the risk sufficiently small (the ember-osf-web frontend will not be affected, at least, and i'd be surprised to see any client that is)

that said, if you disagree i'd be fine to undo it -- the change isn't necessary (so versioning seems excessive), but it seemed better api practice to omit relationships that we can easily tell won't be accessible

@aaxelb aaxelb changed the base branch from develop to feature/insti-dash-improv August 19, 2024 16:32
@mfraezz
Copy link
Member

mfraezz commented Aug 19, 2024

I defer to @brianjgeiger about API versioning, but I do agree that it's pretty low-risk. I'd also be surprised if it does break any clients.

@brianjgeiger
Copy link
Collaborator

Yeah, I don't think we need to version for that. They're undocumented endpoints, and it only affects folks that wouldn't be able to access them anyways.

@aaxelb aaxelb merged commit 5bf4ee4 into CenterForOpenScience:feature/insti-dash-improv Aug 20, 2024
6 checks passed
@aaxelb aaxelb deleted the eng-6077 branch August 20, 2024 14:21
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.

3 participants