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

Use bottom_position_in_list instead of the highest value in the table #266

Merged
merged 1 commit into from
Apr 2, 2017

Conversation

brendon
Copy link
Owner

@brendon brendon commented Apr 1, 2017

Fixes #264. @zharikovpro could you chime in on this one?

I can't see a good reason to have a unique constraint on the position column because unless you're using it without a scope, there will be duplicates. If you're using it without a scope, will my change in this PR still work for you? Could you test it out for me? The tests still pass, and instead of grabbing the MAXIMUM value via SQL, we're just grabbing the last item in the scoped list and adding 2 to that.

I'm not sure if we're testing for this scenario though a cursory glance at your original PR seems to indicate we are.

@yatish27 can you let me know if this works better for you?

@brendon brendon requested a review from swanandp April 1, 2017 09:49
@zharikovpro
Copy link
Contributor

@brendon, sure.

I can't see a good reason to have a unique constraint on the position column

The reason is simple - to have consistent data with validation at the database level.

unless you're using it without a scope, there will be duplicates

It is true that with scope there will be duplicates in one column. However, unique constraint can be composed of 2 columns (say, user and position) to validate that there's no duplicates within that scope.

Could you test it out for me? The tests still pass

If tests are green, then it's just fine. Also, bottom_position_last is perfectly aligned with my initial intent. I only missed that lock issue with maximum, sorry.

P.S. To speed things up, position column could be indexed with or without scope column.

@brendon
Copy link
Owner Author

brendon commented Apr 2, 2017

Thanks @zharikovpro, I'll merge this change then :D I appreciate you taking the time to review this.

@brendon brendon merged commit 0423da7 into master Apr 2, 2017
@brendon brendon deleted the Avoid-maximum branch April 2, 2017 20:55
@zharikovpro
Copy link
Contributor

No problem, this one was easy :)

@swanandp
Copy link
Contributor

swanandp commented Apr 3, 2017

This was easy, and simple. 👍

@swanandp swanandp removed their request for review July 20, 2017 05:30
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.

3 participants