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

Add sorting date to dataSets #3124

Merged
merged 16 commits into from Sep 11, 2018

Conversation

@youri-k
Copy link
Contributor

commented Aug 28, 2018

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • api/datasets should show sortingKey attribute which is similar to the created field

Issues:


@youri-k youri-k self-assigned this Aug 28, 2018

@youri-k youri-k requested review from fm3 and philippotto Aug 28, 2018

youri-k added 2 commits Aug 28, 2018
@youri-k

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2018

@philippotto This is currently only set automatically on dataset creation and therefore has no effects to the current behaviour. Would you be able to do the necessary frontend changes once you're back?

@youri-k youri-k requested a review from rschwanhold Aug 28, 2018

@fm3
fm3 approved these changes Aug 30, 2018
Copy link
Member

left a comment

LGTM

@philippotto philippotto changed the title add sorting key to backend and db [WIP] add sorting key to backend and db Sep 3, 2018

fm3 and others added 5 commits Sep 3, 2018
@MichaelBuessemeyer

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@philippotto could you please check my frontend changes 😃 ?

@philippotto philippotto changed the title [WIP] add sorting key to backend and db Add sorting key to backend and db Sep 10, 2018

@@ -121,6 +121,14 @@ export default class ImportGeneralComponent extends React.PureComponent<Props, S
</FormItemWithInfo>
</Col>
</Row>
<FormItemWithInfo
label="Publication Date"
info="This date is the publication date of the dataset. It is used to sort the datasets in the dataset gallery."

This comment has been minimized.

Copy link
@philippotto

philippotto Sep 10, 2018

Member

I'd avoid "publication date" here as this might make users think that the dataset is public.

How about:

label: "Sorting Date"
info: "This date can be used to sort the datasets within webKnossos. For example, if the dataset was published in a paper, you can input the publication date of the paper here."

@normanrz @daniel-wer Do you agree?

This comment has been minimized.

Copy link
@MichaelBuessemeyer

MichaelBuessemeyer Sep 10, 2018

Contributor

We already had a discussion about this naming issue. But I think your solution is more elegant and forward.

@philippotto

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Thanks, @MichaelBuessemeyer! This PR should be good to go. @youri-k Can you do the merge + deployment since it requires a migration + datastore update?

@MichaelBuessemeyer

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@philippotto I still got some flow errors (dont know why the showed up so late :/ ) => flow backend-type checks are not correct: I still have to modify app/assets/javascripts/test/snapshots/flow-check/test-flow-checking-dataset.js

Edit

never mind, they just also magically vanished the ways they appeared

@philippotto

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I still got some flow errors (dont know why the showed up so late :/ )

Within our e2e tests, we dynamically create JS files whose sole purpose is to validate whether the front-end types are "in sync" with the data provided by the back-end types. That's why, some flow errors only turn up after running the e2e tests. In this case, I assume that the api dataset type doesn't have the sortingKey?

@daniel-wer

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

@MichaelBuessemeyer As the e2e tests ran through, you should be good :)
The files in the flow-check subdir are only created after running the e2e-tests to check whether the backend objects are conforming to our frontend flow types. Unfortunately we are not deleting those files when switching branches, so sometimes the files are remaining from another branch and cause "non-existent" flow errors. We'll have to think of a solution for this, maybe deleting the flow-check dir when switching branches or so.

@youri-k youri-k changed the title Add sorting key to backend and db Add sorting date to dataSets Sep 11, 2018

@youri-k youri-k merged commit 7d7723b into master Sep 11, 2018

2 checks passed

ci/circleci: build_test_deploy Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 51.097%
Details

@fm3 fm3 deleted the dataset-gallery-enhancement branch Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.