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-6242] Resolve issue with updating preprint fields and validation errors #10758

Merged

Conversation

uditijmehta
Copy link
Contributor

@uditijmehta uditijmehta commented Sep 20, 2024

Purpose

Fix form validation issues in the preprint model and serializer.

Changes

  • Updated preprint model to correctly handle validation.
  • Modified serializer logic to properly manage field validation.

Ticket

ENG-6242

@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch 5 times, most recently from 5471536 to 884ef60 Compare September 20, 2024 21:18
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.

I think the changes to the model are good ones, but I think that we are risking losing data from API requests silently if we go with this as a complete solution. There'll need to be some modifications made to the tests and to the serializer functions, I believe.

api_tests/preprints/views/test_preprint_detail.py Outdated Show resolved Hide resolved
api_tests/preprints/views/test_preprint_detail.py Outdated Show resolved Hide resolved
osf/models/preprint.py Show resolved Hide resolved
api/preprints/serializers.py Outdated Show resolved Hide resolved
@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch 2 times, most recently from 564bada to 909fee9 Compare October 3, 2024 17:56
@brianjgeiger brianjgeiger changed the base branch from feature/b-and-i-24-14 to master October 4, 2024 12:56
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.

Don't merge this, because it's a hotfix and we'll be going through that workflow for it.

It's not the way I expected this to be resolved, but it has the advantage of clearing out bad data if someone uses the old workflow that the original preprints app used, where the newly emptied fields would just not be included in the payload.

@brianjgeiger
Copy link
Collaborator

@uditijmehta Could you resolve the conflicts by merging master into this branch?

@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch 3 times, most recently from eaaca90 to 83b4c7e Compare October 4, 2024 17:13
@uditijmehta uditijmehta changed the base branch from master to develop October 4, 2024 18:02
@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch 5 times, most recently from 17ccc19 to ec69be1 Compare October 4, 2024 19:30
@cslzchen
Copy link
Collaborator

cslzchen commented Oct 7, 2024

Late to the CR party, one suggestion and one potential blocker?

  • I think we can remove handle_author_assertions() since we have removed the only usage of it.
  • One crucial part in handle_author_assertions() was not ported to the "expanded" if statements: admin permission check. This is not testable by QA due to the assertion page disabled by FE but I think WRITE contributor can use API to change it. Probably a blocker?

@brianjgeiger
Copy link
Collaborator

@cslzchen So we need to make sure to require admin permissions to do author assertions before we move forward is the suggestion?

@cslzchen
Copy link
Collaborator

cslzchen commented Oct 7, 2024

Yes, I think so.

@brianjgeiger
Copy link
Collaborator

@uditijmehta Could you make that change, and also add a test for that case?

@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch 2 times, most recently from bb320a3 to 3b55a71 Compare October 9, 2024 14:50
@brianjgeiger
Copy link
Collaborator

New changes seem good to me. @cslzchen?

@cslzchen
Copy link
Collaborator

  • I see we have two parts of the same/similar check, one is R344-R410 with the admin check patch while the other is R491-R551 without the admin patch. Maybe duplicate / left-over code?

  • In addition, we should remove this method def handle_author_assertions(self, preprint, validated_data, auth): ...

@uditijmehta uditijmehta force-pushed the hotfix/form-validation-issues branch from f41e191 to 8c599e6 Compare October 10, 2024 14:51
Copy link
Collaborator

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

LGTM 🎆

@cslzchen cslzchen changed the base branch from develop to master October 16, 2024 14:42
@cslzchen cslzchen force-pushed the hotfix/form-validation-issues branch from 8c599e6 to 59fab7d Compare October 21, 2024 15:03
@cslzchen
Copy link
Collaborator

I cherry-picked 6 commits (and force pushed back) since the original branch has develop changes which should not be released to production.

Original branch has been backed up in https://github.com/cslzchen/osf.io/tree/hotfix/form-validation-issues-backup.

cslzchen added a commit that referenced this pull request Oct 21, 2024
Resolve issue with updating preprint fields and validation errors

* [ENG-6242] #10758
@cslzchen cslzchen merged commit c0eb27b into CenterForOpenScience:master Oct 21, 2024
6 checks passed
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