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

Prevent mention form from hanging forever #1393

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

ewan-escience
Copy link
Collaborator

Prevent mention form from hanging forever

Changes proposed in this pull request

  • When parsing a Crossref mention, check if the title exists
  • Show an error message when something is wrong with a mention
  • Remove the unused function getUrlFromDoiOrg
  • Add a link to some lists of DOIs in the README for developers

How to test

  • docker compose down --volumes && docker compose build --parallel && docker compose up --scale data-generation=0
  • Sign in as admin, create a software or project page
  • Add as mention ​https://doi.org/10.61686/GOLUA29851, this should result in an error message concerning its missing title
  • Bulk import wasn't changed, this should still result in Unknown error
  • Check the README

Part of #1384

PR Checklist:

  • Increase version numbers in docker-compose.yml
  • Link to a GitHub issue
  • Update documentation
  • Tests

@ewan-escience ewan-escience self-assigned this Feb 7, 2025
return crossrefItemToMentionItem(item)
try {
return crossrefItemToMentionItem(item)
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove e, it is unused

try{
  ... 
} catch {
  return null
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What should I put instead? It has to have a name and ESLint also complains when I use _.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean change catch(e) to just catch because e is not used.
The rest of the code remains unchanged.

Comment on lines -77 to -96
export async function getUrlFromDoiOrg(doi: string) {
try {
const url = ` https://doi.org/api/handles/${encodeURIComponent(doi)}?type=URL`
const resp = await fetch(url)
// debugger
if (resp.status === 200) {
const json: DoiUrlResponse = await resp.json()
// extract
if (json.values.length > 0) {
const item = json.values[0]
if (item.type.toLowerCase() === 'url') {
return item.data.value
}
}
}
} catch (e: any) {
logger(`getUrlFromDoiOrg: ${e?.message}`, 'error')
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also remove code from line 19-37 which is associated with this method

@@ -59,8 +61,12 @@ export default function FindMentionSection({id,config,findPublicationByTitle}:Fi
const resp = await getMentionByDoi(searchFor)
if (resp?.status === 200) {
return [resp.message as MentionItemProps]
} else {
if (resp?.message) {
showErrorMessage(resp.message)
Copy link
Contributor

@dmijatovic dmijatovic Feb 7, 2025

Choose a reason for hiding this comment

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

Maybe extend/combine error messages. Currently the message is about missing title.
I think it would be more clear to say: "Failed to import/process mention. \n{err.message}"
Something like that.

Copy link
Contributor

@dmijatovic dmijatovic left a comment

Choose a reason for hiding this comment

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

It works as described. I have few minor remarks.

@ewan-escience ewan-escience force-pushed the 1384-fix-mention-form-no-title branch from 96295b6 to 09d57b0 Compare February 7, 2025 16:38
Copy link

sonarqubecloud bot commented Feb 7, 2025

@ewan-escience ewan-escience merged commit 246c779 into main Feb 10, 2025
5 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.

2 participants