-
Notifications
You must be signed in to change notification settings - Fork 0
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
Multiple posting of full PREreview #400
Comments
@jheretic sorry to bug you about this, but do you know why this is happening? |
A reviewer said they got an error (Failed to fetch: 400 Bad Request) when trying to submit their full PREreview. |
Hey all, I'll take a look as soon as I can! Having a staff retreat this week but I'll look at it in the off hours and let you know! |
Thank you @jheretic. We just got another similar report. "I just wrote my first full prereview (yay!) but got a 400 error popup when I tried to submit it. I clicked again (same error) and then went to my profile and saw the review did get received and has been posted twice. Can you remove one of them please (I also clicked report on one but expected to have the option to more details) https://prereview.org/preprints/doi-10.1101-2020.12.11.422154 |
@dasaderi Copied from Slack: it appears that the HTTP 400 error is because it's failing to fetch a DOI from Zenodo for the review. It's not clear yet why that's failing (I'm setting up some test code to examine the review cycle to see if it's an issue on their side or ours), but as it stands that is setup to cause the review submission to be denied. That should cause it to cancel out of the request, but for some reason the review is still getting commited to the database in a way that causes the author check to be skipped. It's not clear why that's getting skipped, but the prereview submission logic is pretty complex because of all the draft and ownership checks. I'll see about making sure the ownership check isn't skipped if the DOI fetch from Zenodo fails. |
@jheretic THANK YOU so much for taking the time to look into this. Do you know or have an idea of why the DOI from Zenodo may not be fetched properly? @thewilkybarkid is out on holiday this week, but he'll take a look too when he's back. Happy to pay for your hours of work Josh if you have time to fix this problem this week. |
Since it's an external service there's always a chance that it'll be unavailable. Reading the code in prereview/src/backend/controllers/fullReview.js Lines 180 to 205 in 03cb583
I haven't any logs to see why step 3 failed, but there's always a chance it will. This should be a distributed transaction (a unit of work involving multiple hosts) but isn't. Instead, each operation is its own transaction, so a failure only stops later ones from happening (rather than also rolling back earlier ones). |
In terms of actions:
Also, what should happen with the published reviews that have no DOI? (Refs sciety/sciety#1142) |
When the code is changed, we might need migration files to ensure the database has the structure that the code expects. The ORM library can generate these, but they have to be manually run and checked. This change adds a check to CI to ensure that they are updated (i.e. the ORM doesn't find any differences). I expect CI to fail, as they are not currently up to date. Refs #388, #395, #400
I've started the (long) road to creating a failing test case for when Zenodo fails. Unfortunately, as the code is not amenable to testing it has to have a real database running (as the code depends on the ORM). This has thrown up a surprise in that the ORM is misconfigured and has unresolved migrations (as it stands, a new DB structure can't be generated from the code). I'm trying to reveal this problem in d4c18e4. |
The database migrations are out of sync with the codebase: 1. The code doesn't know about some of the indexes in the migrations. 2. The code has indexes that don't exist in the migrations. 3. The code has misconfigured indexes; the ORM cannot read them but doesn't error until it tries to apply the broken SQL. These problems caused a failure in CI as it now checks that no migrations are missing. This change removes the broken and unapplied changes and adds in code for the missing ones to allow CI to pass. I'm not sure if any of the valid but unapplied indexes would help with performance issues, but they can be revisited later. Refs #388, #395, #400, d4c18e4
The next big hurdle for testing is to make the configuration amenable (#419). |
Been taking a look into duplicate reviews, and noticed that when a full review is published a new one is made rather than updating the This partially explains why https://prereview.org/api/v2/preprints/8429c125-783e-47b2-974f-fa47863c0bd2 shows three full reviews by the same author (2 unpublished, 1 published). |
Some users are posting multiple full PREreviews per preprint. There should be only one full PREreview per user allowed.
E.g. https://prereview.org/preprints/doi-10.21203-rs.3.rs+420780-v1
The text was updated successfully, but these errors were encountered: