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.

Sign up for GitHub

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

Jump to bottom

Fix File show_as_unviewed behavior #9960

Merged
merged 25 commits into from
Jun 21, 2022
Merged

Fix File show_as_unviewed behavior #9960

merged 25 commits into from
Jun 21, 2022

Conversation

jwalz
Copy link
Contributor

@jwalz jwalz commented Jun 21, 2022

Purpose

The legacy behavior imitated by the current_user_has_seen attribute wasn't quite as simple as the name suggested.
The FE only wanted to mark a File as unseen in the case where the user hadn't seen the latest version and they had viewed a previous version.

Changes

  • Rename the attribute from current_user_has_seen to show_as_unviewed to more accurately reflect the intent and be less misleading
  • Implement updated logic using annotations to avoid overly-expensive SerializerMethodField behavior
  • Update tests to be more thorough and co-locate them with relevant endpoint tests

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

Jon Walz added 24 commits June 16, 2022 09:48
Try annotating current_user_has_seen
f317561
Update serializer (expecting test fails)
499c332
Some fixes
94837c0
Fix bad file_id reference
718cf91
Moar syntax
6aff754
Moar file_id
2ec3d3d
more mucking
586240b
Move final annotation into Case expression
c93d116
Add output_type to case; tentative function for annotiations
2a29b3a
Change current_user_has_viewed to show_as_unviewed; move logic into
d1669ab 
module
missed a spot
b2357e4
Annotation syntax errors + tests
7af05bc
Test cleanup
3521c40
Fix tests, move annotation
58dbd24
Footguns
53052fb
Point tests at correct endpoints...
e0597c3
Better testing for non-OsfStorageFiles; get rid of trailing comma from
2fde143 
hell
Fix the correct comma
6c68284
Final(?) test fixes
c64d849
:(
7d47a63
Move tests to the related endpoint files
f2e9627
Moar dumb test errors
268c357
Bad self references in tests
bbf439d
Always apply show_as_unviewed on FileDetail endpoints instead of
c5173cb 
relying on default values
brianjgeiger
brianjgeiger requested changes Jun 21, 2022
View reviewed changes
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.

Only the tiniest of typos on one of the docstrings.

api/files/annotations.py Outdated Show resolved Hide resolved
brianjgeiger
brianjgeiger approved these changes Jun 21, 2022
View reviewed changes
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.

GHA takes forever to run. I'm not going to hold up the PR for a typo, but it wouldn't be bad to fix if there turns out to be anything else.

@jwalz @brianjgeiger
Update api/files/annotations.py
be44698 
Co-authored-by: Brian J. Geiger <[email protected]>
@jwalz jwalz merged commit 8052d1e into CenterForOpenScience:develop Jun 21, 2022
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jun 26, 2022
Merge branch 'feature/django_upgrade' of https://github.com/CenterFor…
76f054c 
…OpenScience/osf.io into load-lincenses

* 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io:
  remove post-migrate signals from migration stream (CenterForOpenScience#9964)
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ensure VOL users always get read perms (CenterForOpenScience#9952)
  make types correspond more to serializer code (CenterForOpenScience#9955)
  remove special casing from DV permissions (CenterForOpenScience#9947)
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jun 26, 2022
Merge branch 'feature/django_upgrade' of https://github.com/CenterFor…
f3d0bbf 
…OpenScience/osf.io into signal-storage-region

* 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io:
  remove post-migrate signals from migration stream (CenterForOpenScience#9964)
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ensure VOL users always get read perms (CenterForOpenScience#9952)
  make types correspond more to serializer code (CenterForOpenScience#9955)
  remove special casing from DV permissions (CenterForOpenScience#9947)
  Add a generic demo institution on the test server
  Don't send empty affiliations list to Datacite or Crossref
  Update pre-commit config to specify py version
  Fix DOI conditional
  Bump version and update CHANGELOG
  Update sc to use new SSO IdP
  [ENG-3832] Add redirect based for unauthed files page user (CenterForOpenScience#9948)
  [ENG-3781] Admin Registration Schemas (CenterForOpenScience#9937)
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jun 27, 2022
Merge branch 'feature/django_upgrade' of https://github.com/CenterFor…
74f70bd 
…OpenScience/osf.io into investigate-waffle-flags

* 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io:
  move createcachetable to post migrate signal (CenterForOpenScience#9944)
  remove post-migrate signals from migration stream (CenterForOpenScience#9964)
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)

# Conflicts:
#	osf/apps.py
#	osf/migrations/0137_transfer_preprint_service_permissions.py
#	osf/migrations/__init__.py
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jun 27, 2022
Merge branch 'feature/django_upgrade' of https://github.com/CenterFor…
6a7f83a 
…OpenScience/osf.io into signal-deny-list

* 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io:
  move createcachetable to post migrate signal (CenterForOpenScience#9944)
  remove post-migrate signals from migration stream (CenterForOpenScience#9964)
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)

# Conflicts:
#	osf/apps.py
#	osf/migrations/0137_transfer_preprint_service_permissions.py
#	osf/migrations/__init__.py
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jun 27, 2022
Merge branch 'feature/django_upgrade' of https://github.com/CenterFor…
578f0ce 
…OpenScience/osf.io into signal-deny-list

* 'feature/django_upgrade' of https://github.com/CenterForOpenScience/osf.io:
  move createcachetable to post migrate signal (CenterForOpenScience#9944)
  remove post-migrate signals from migration stream (CenterForOpenScience#9964)
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)

# Conflicts:
#	osf/apps.py
#	osf/migrations/0137_transfer_preprint_service_permissions.py
#	osf/migrations/__init__.py
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jul 29, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
9ec5802 
 into fix-mailchimp

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (73 commits)
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ensure VOL users always get read perms (CenterForOpenScience#9952)
  make types correspond more to serializer code (CenterForOpenScience#9955)
  remove special casing from DV permissions (CenterForOpenScience#9947)
  Add a generic demo institution on the test server
  Don't send empty affiliations list to Datacite or Crossref
  Update pre-commit config to specify py version
  Fix DOI conditional
  Bump version and update CHANGELOG
  Update sc to use new SSO IdP
  ...

# Conflicts:
#	website/templates/profile/notifications.mako
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jul 29, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
76e3d5f 
 into add-doi-domain

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (126 commits)
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ensure VOL users always get read perms (CenterForOpenScience#9952)
  make types correspond more to serializer code (CenterForOpenScience#9955)
  remove special casing from DV permissions (CenterForOpenScience#9947)
  Add a generic demo institution on the test server
  Don't send empty affiliations list to Datacite or Crossref
  Update pre-commit config to specify py version
  Fix DOI conditional
  Bump version and update CHANGELOG
  Update sc to use new SSO IdP
  ...
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Aug 16, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
5845010 
 into remove-quickfiles-code

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (44 commits)
  Re-add non-anonymized fields removed in CenterForOpenScience#10009 (CenterForOpenScience#10022)
  Update shield logo for colorado (UC Boulder)
  Update description for maglab
  [ENG-3249] Improve registration anonymization (CenterForOpenScience#10009)
  "backport" artifact changes and swap schema_response.justification (CenterForOpenScience#10003)
  Add new instn purdue
  Ensure BitBucket token is string, not bytes
  Only redirect to cas if not logged in
  OSFI: Update Shared SSO and Add MagLab/FSU [ENG-3654]
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ...

# Conflicts:
#	api_tests/files/views/test_file_detail.py
#	osf/models/nodelog.py
#	osf/models/user.py
#	tests/test_addons.py
#	website/static/js/anonymousLogActionsList.json
#	website/static/js/logActionsList.json
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Aug 16, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
ce4c186 
 into remove-quickfiles-code

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (44 commits)
  Re-add non-anonymized fields removed in CenterForOpenScience#10009 (CenterForOpenScience#10022)
  Update shield logo for colorado (UC Boulder)
  Update description for maglab
  [ENG-3249] Improve registration anonymization (CenterForOpenScience#10009)
  "backport" artifact changes and swap schema_response.justification (CenterForOpenScience#10003)
  Add new instn purdue
  Ensure BitBucket token is string, not bytes
  Only redirect to cas if not logged in
  OSFI: Update Shared SSO and Add MagLab/FSU [ENG-3654]
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ...

# Conflicts:
#	api_tests/files/views/test_file_detail.py
#	osf/models/nodelog.py
#	osf/models/user.py
#	tests/test_addons.py
#	website/static/js/anonymousLogActionsList.json
#	website/static/js/logActionsList.json
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Aug 16, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
c29881a 
 into remove-quickfiles-code

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (44 commits)
  Re-add non-anonymized fields removed in CenterForOpenScience#10009 (CenterForOpenScience#10022)
  Update shield logo for colorado (UC Boulder)
  Update description for maglab
  [ENG-3249] Improve registration anonymization (CenterForOpenScience#10009)
  "backport" artifact changes and swap schema_response.justification (CenterForOpenScience#10003)
  Add new instn purdue
  Ensure BitBucket token is string, not bytes
  Only redirect to cas if not logged in
  OSFI: Update Shared SSO and Add MagLab/FSU [ENG-3654]
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ...

# Conflicts:
#	api_tests/files/views/test_file_detail.py
#	osf/models/nodelog.py
#	osf/models/user.py
#	tests/test_addons.py
#	website/static/js/anonymousLogActionsList.json
#	website/static/js/logActionsList.json
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Aug 16, 2022
Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io
1341f59 
 into remove-quickfiles-code

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (44 commits)
  Re-add non-anonymized fields removed in CenterForOpenScience#10009 (CenterForOpenScience#10022)
  Update shield logo for colorado (UC Boulder)
  Update description for maglab
  [ENG-3249] Improve registration anonymization (CenterForOpenScience#10009)
  "backport" artifact changes and swap schema_response.justification (CenterForOpenScience#10003)
  Add new instn purdue
  Ensure BitBucket token is string, not bytes
  Only redirect to cas if not logged in
  OSFI: Update Shared SSO and Add MagLab/FSU [ENG-3654]
  [ENG-3898][ENG-3899]Model support for OutcomeArtifact update and delete (CenterForOpenScience#9989)
  Make OutcomeArtifact.identifier nullable (CenterForOpenScience#9986)
  [ENG-3894] Outcome models (CenterForOpenScience#9975)
  revert color picker to working version (CenterForOpenScience#9968)
  Instrument the ORCiD SSO affiliation flow * Existing user with verified ORCiD ID * Existing user confirmation of linking ORCiD ID * New user confirmation of account creation with ORCiD ID
  Add a django command script to handle instn sso email domain changes
  Bump version and CHANGELOG
  Add better logic for routing Dataverse files and their stupid duplicate (CenterForOpenScience#9963)
  [ENG-3872] Fix dataverse 502s and pass version info to FE (CenterForOpenScience#9959)
  Fix File `show_as_unviewed` behavior (CenterForOpenScience#9960)
  Fix OSFUser.has_resources
  ...

# Conflicts:
#	api_tests/files/views/test_file_detail.py
#	osf/models/nodelog.py
#	osf/models/user.py
#	tests/test_addons.py
#	website/static/js/anonymousLogActionsList.json
#	website/static/js/logActionsList.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brianjgeiger brianjgeiger brianjgeiger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jwalz @brianjgeiger