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.

Sign up for GitHub

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

Jump to bottom

Display name for organizations #2869

Merged
merged 9 commits into from
Jul 10, 2018
Merged

Display name for organizations #2869

merged 9 commits into from
Jul 10, 2018

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Jul 9, 2018 •
edited
Loading

Steps to test:

  • call api/organizations and see displayName property

Issues:

@youri-k youri-k added this to the Sprint 24b milestone Jul 9, 2018
@youri-k youri-k self-assigned this Jul 9, 2018
@youri-k youri-k requested review from fm3 and philippotto July 9, 2018 13:40
@youri-k
Copy link
Contributor Author

youri-k commented Jul 9, 2018

@philippotto you can merge this into onboarding whenever you want if @fm3 's review is positive.
I also don't know where this change impacts the frontend, so you may be able to adapt it properly

fm3
fm3 approved these changes Jul 9, 2018
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

LGTM + works for me.
I added the backwards evolution and adapted the test db

youri-k
youri-k commented Jul 9, 2018
View reviewed changes
conf/evolutions/reversions/003-add-organization-displayname.sql Outdated
@@ -0,0 +1,5 @@
START TRANSACTION;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I just forgot to change the name of the evolution, so we should remove one of the files

Copy link
Member

Choose a reason for hiding this comment

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

agree. @jstriebel should the CI check have caught this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly? The CI only checks the forward-evolutions, to prevent using the same number with merges. See https://github.com/scalableminds/webknossos/blob/master/tools/postgres/assert_unique_evolution_numbers.sh

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks

@philippotto philippotto merged commit c39c1b1 into onboarding Jul 10, 2018
@fm3 fm3 deleted the display-name-for-organizations branch July 12, 2018 09:30
@philippotto philippotto mentioned this pull request Jul 12, 2018
Merged
7 tasks
@fm3 fm3 mentioned this pull request Jul 12, 2018
Closed
@normanrz normanrz mentioned this pull request Jul 13, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstriebel jstriebel jstriebel left review comments

@fm3 fm3 fm3 approved these changes

@philippotto philippotto Awaiting requested review from philippotto

Assignees

@youri-k youri-k

Labels
backend enhancement
Projects
None yet
Milestone
Sprint 24b
Development

Successfully merging this pull request may close these issues.
4 participants
@youri-k @fm3 @jstriebel @philippotto