-
Notifications
You must be signed in to change notification settings - Fork 351
[ENG-9651] Fix 502s when trying to make it public #11414
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
base: feature/pbs-25-21
Are you sure you want to change the base?
[ENG-9651] Fix 502s when trying to make it public #11414
Conversation
osf/models/node.py
Outdated
| try: | ||
| if self.get_identifier_value('doi'): | ||
| update_doi_metadata_on_change(self._id) | ||
| elif self.is_registration: | ||
| doi = self.request_identifier('doi')['doi'] | ||
| self.set_identifier_value('doi', doi) | ||
| except Exception: | ||
| logger.exception( | ||
| f'Failed to create/update DOI for {self._id} during set_privacy. ' | ||
| ) |
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.
Should we allow the DOI creation process to be skipped?
I recommend at least a pop-up with additional confirmation from the admins before skipping the DOI creation.
@brianjgeiger @adlius
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.
I expect Product would rather have the doi creation succeed. It would at least be good to know what went wrong during doi creation to cause it to fail, so if we need to do anything else here to get the appropriate information to sentry, we should do that.
Plus, this try block looks not correct. This exception catch should happen around L1242 rather than the whole block. There should be a different try block around L1239 that indicates that we weren't able to update metadata for the guid.
Ostap-Zherebetskyi
left a comment
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.
The code looks good, but we need to verify whether we want to allow the DOI creation process to be skipped.
40997dd to
ab8ca9c
Compare
Ostap-Zherebetskyi
left a comment
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.
LGTM
Purpose
Fix 502s when trying to make it public
Changes
Add exception for DOI creation
QA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket
https://openscience.atlassian.net/browse/ENG-9651?atlOrigin=eyJpIjoiMGM1NGJkY2ZiOWQ0NGM1MjkwMjk2MDg0N2QyM2Q1OWEiLCJwIjoiaiJ9