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

Only upload to datastores that allow upload #5404

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Apr 16, 2021

Fixed a bug where dataset uploads were sent to the wrong datastore (first in list, regardless of allowsUpload), and thus failed.

I’m a bit confused why this bug didn’t occur before. The datastore list from the server is ordered by name, so the “correct” one, webknossos.org is sent last, but the old code took the first. regression introduced in #5350 (for explanation see comment below)

Steps to test:

  • set up external datastore, insert into db with allowsUpload = false
  • upload dataset, should go to local datastore
  • toggle allowsUpload for both datastes
  • reload upload page, upload should now go to the other datastore.

@fm3 fm3 added the bug label Apr 16, 2021
@fm3 fm3 self-assigned this Apr 16, 2021
@fm3 fm3 added the urgent label Apr 16, 2021
@fm3 fm3 requested a review from philippotto April 16, 2021 12:47
@MichaelBuessemeyer
Copy link
Contributor

I just had a quick look at the previous versions of the code:
There are / were at least two versions with that bug.
The most recent version came with the antd upgrade and is currently deployed on the master.

Before the antd upgrade, the code was:

 static getDerivedStateFromProps(props) {
    if (
      props.datastores.length === 1 &&
      props.form.getFieldValue("datastore") !== props.datastores[0].url
    ) {
      props.form.setFieldsValue({ datastore: props.datastores[0].url });
    }
    return null;
  }

As you can see, I switched from getDerivedStateFromProps to componentDidUpdate during the antd upgrade.
One significant reason why the bug now occurred on the master might be, that previous to the antd upgrade's version the datastore field in the form was only updated, if there was exactly one datastore. During the antd upgrade, I changed this to always select a datastore once at least one is available. And this is pretty likely the reason why the bug was only present on wk.org with the antd upgrade because wk.org has more than just one datastore.

@fm3 fix should work imo as it does the required filtering for datastores that allow uploads.

@fm3 fm3 enabled auto-merge (squash) April 16, 2021 13:11
@fm3 fm3 merged commit 70cdf44 into master Apr 16, 2021
@fm3 fm3 deleted the uploadable-datastore branch May 3, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants