Improved sortable behavior #359

Merged
merged 9 commits into from Aug 22, 2012

Conversation

Projects
None yet
4 participants
@rozwell
Contributor

rozwell commented May 11, 2012

Changes:

  • added native moving between scopes when scope value was changed and rank not (if rank was changed - we assume superior change)
  • regular saving objects with empty/null scope was setting correct rank but methods like insertAtBottom() didn't allow inserting in null scope (lack of consistency?)
    It is now possible to use those methods in null scope (why not?).
    See: native sorting in sfPropelORMPlugin admin generator
  • removeFromList() was clearing both, scope and rank column and breaking good structure in null scope.
    Now it moves object to the end of null scope
  • swapWith() can now fully operate between scopes - previously it would only exchange ranks, which was fine for the same scope, but would break the structure in both scopes (no exception was thrown)

There are some other issues like when scope column is a FK and onDelete="setnull", removing related/scope object will only clean the scope value leaving ranks untouched.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 11, 2012

This pull request fails (merged 07ff600 into 82372dd).

This pull request fails (merged 07ff600 into 82372dd).

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell May 11, 2012

Contributor

@travisbot You're right, I will have to adjust and add tests too.
But lets wait for someone to comment if this is a good way to go or shall we change something else?

Edit: Why does removeFromList() sets rank to null but there is no "AND SORTABLE_RANK IS NOT NULL" condition in any query?
Especially in non-scope tables where this could be expected(?)

This behavior needs more solid concept.
For now I'm confused.

Contributor

rozwell commented May 11, 2012

@travisbot You're right, I will have to adjust and add tests too.
But lets wait for someone to comment if this is a good way to go or shall we change something else?

Edit: Why does removeFromList() sets rank to null but there is no "AND SORTABLE_RANK IS NOT NULL" condition in any query?
Especially in non-scope tables where this could be expected(?)

This behavior needs more solid concept.
For now I'm confused.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 12, 2012

This pull request fails (merged 9df7198 into 82372dd).

This pull request fails (merged 9df7198 into 82372dd).

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand May 12, 2012

Member

@rozwell travisbot is... a bot :p

Your work seems ok, but I don't really know this behavior so.. let's see other comments, and try to fix the test suite.

Member

willdurand commented May 12, 2012

@rozwell travisbot is... a bot :p

Your work seems ok, but I don't really know this behavior so.. let's see other comments, and try to fix the test suite.

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell May 12, 2012

Contributor

@willdurand I know ;)
Unit tests are easy to fix (just 2 failed) and I will add a few more too if these changes are ok.

Contributor

rozwell commented May 12, 2012

@willdurand I know ;)
Unit tests are easy to fix (just 2 failed) and I will add a few more too if these changes are ok.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand May 12, 2012

Member

It looks fine to me, but as I said, I'm not expert on this. Maybe we should try to expose what the behavior should do, and how before to change everything. What do you think?

Member

willdurand commented May 12, 2012

It looks fine to me, but as I said, I'm not expert on this. Maybe we should try to expose what the behavior should do, and how before to change everything. What do you think?

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell May 12, 2012

Contributor

I took one more look at this behavior and after my modifications we actually need to talk about those 2 things only:

  1. removeFromList() - I'd suggest to remove this method since we can just change the scope to null or if scope isn't used it seems better to add other column to manage visibility like is_visible so there is no need for adding NOT NULL condition mentioned above.
    I've got in mind admin generator so this may affect my judgment since I also see how to manage null ranks.
  2. onDelete="setnull" - removing related object will only clean the scope value leaving ranks untouched and since we sort in null scope too, shouldn't they be moved at the end of it? (makes most sense)
Contributor

rozwell commented May 12, 2012

I took one more look at this behavior and after my modifications we actually need to talk about those 2 things only:

  1. removeFromList() - I'd suggest to remove this method since we can just change the scope to null or if scope isn't used it seems better to add other column to manage visibility like is_visible so there is no need for adding NOT NULL condition mentioned above.
    I've got in mind admin generator so this may affect my judgment since I also see how to manage null ranks.
  2. onDelete="setnull" - removing related object will only clean the scope value leaving ranks untouched and since we sort in null scope too, shouldn't they be moved at the end of it? (makes most sense)
@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Jun 22, 2012

Member

I think you are changing the behavior of ... this behavior :)

  1. I mean, a null scope should not mean objects are not in a list. The "null" scope is actually a scope, like a "global" scope, or a "default" one I guess. So the removeFromList() makes sense. But I agree with the is_visible, it could a nice option.
  2. You're right!

Sorry for the delay, I tried to learn this behavior a bit more.

Member

willdurand commented Jun 22, 2012

I think you are changing the behavior of ... this behavior :)

  1. I mean, a null scope should not mean objects are not in a list. The "null" scope is actually a scope, like a "global" scope, or a "default" one I guess. So the removeFromList() makes sense. But I agree with the is_visible, it could a nice option.
  2. You're right!

Sorry for the delay, I tried to learn this behavior a bit more.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 12, 2012

This pull request fails (merged fb30d63 into 8b02639).

This pull request fails (merged fb30d63 into 8b02639).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 12, 2012

This pull request fails (merged 5b06917 into 8b02639).

This pull request fails (merged 5b06917 into 8b02639).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 12, 2012

This pull request fails (merged cce76e3d into 8b02639).

This pull request fails (merged cce76e3d into 8b02639).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 12, 2012

This pull request fails (merged 957b90a into 8b02639).

This pull request fails (merged 957b90a into 8b02639).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 12, 2012

This pull request fails (merged 7e169b3 into 8b02639).

This pull request fails (merged 7e169b3 into 8b02639).

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 12, 2012

Contributor

@willdurand is there any reason to have boolean _use_scope_ parameter for scope, when you have to set _scope_column_ anyway?
Isn't setting _scope_column_ enough?

Contributor

rozwell commented Aug 12, 2012

@willdurand is there any reason to have boolean _use_scope_ parameter for scope, when you have to set _scope_column_ anyway?
Isn't setting _scope_column_ enough?

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Aug 13, 2012

Member

well, I think you can enable/disable the use of scopes easily.

Member

willdurand commented Aug 13, 2012

well, I think you can enable/disable the use of scopes easily.

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Aug 15, 2012

Member

This PR needs unit tests that fail if the builder is not modified and passes otherwise - this will reveal the interest of tour modification.

Member

fzaninotto commented Aug 15, 2012

This PR needs unit tests that fail if the builder is not modified and passes otherwise - this will reveal the interest of tour modification.

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 16, 2012

Contributor

@fzaninotto can you explain with more details?

Contributor

rozwell commented Aug 16, 2012

@fzaninotto can you explain with more details?

rozwell added some commits Aug 16, 2012

sortable behavior: tests for new scope approach (includes fixed typo …
…in SortableBehaviorObjectBuilderModifierWithScopeTest::testInsertAtRank())
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 16, 2012

This pull request passes (merged 3af6a72 into 441f969).

This pull request passes (merged 3af6a72 into 441f969).

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 16, 2012

Contributor

Little summary of changes:

  • added native moving between scopes when scope value was changed and rank not (if rank was changed - we assume superior change)
  • methods like insertAtBottom() now works with null scope
  • removeFromList() now moves object to the end of null scope (if scope is used)
  • swapWith() can now fully operate between scopes

TODO:
When scope column is a FK and onDelete="setnull", removing related object should move all objects to null scope (maybe trigger
_removeFromList()_ ?) - is there a proper way to do so?

Contributor

rozwell commented Aug 16, 2012

Little summary of changes:

  • added native moving between scopes when scope value was changed and rank not (if rank was changed - we assume superior change)
  • methods like insertAtBottom() now works with null scope
  • removeFromList() now moves object to the end of null scope (if scope is used)
  • swapWith() can now fully operate between scopes

TODO:
When scope column is a FK and onDelete="setnull", removing related object should move all objects to null scope (maybe trigger
_removeFromList()_ ?) - is there a proper way to do so?

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 19, 2012

Contributor

Seems ready to be merged.
Any thoughts?

Contributor

rozwell commented Aug 19, 2012

Seems ready to be merged.
Any thoughts?

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Aug 20, 2012

Member

So null is a scope then?

Member

willdurand commented Aug 20, 2012

So null is a scope then?

@fzaninotto

This comment has been minimized.

Show comment
Hide comment
@fzaninotto

fzaninotto Aug 22, 2012

Member

ok, I'm +1. Good patch.

Member

fzaninotto commented Aug 22, 2012

ok, I'm +1. Good patch.

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 22, 2012

Contributor

@willdurand yes sir.

Contributor

rozwell commented Aug 22, 2012

@willdurand yes sir.

willdurand added a commit that referenced this pull request Aug 22, 2012

@willdurand willdurand merged commit 19dc671 into propelorm:master Aug 22, 2012

1 check passed

default The Travis build passed
Details
@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Aug 22, 2012

Member

Great, merged! Thank you so much.

Member

willdurand commented Aug 22, 2012

Great, merged! Thank you so much.

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Aug 22, 2012

Member

Can you port this PR on Propel2? :)

Member

willdurand commented Aug 22, 2012

Can you port this PR on Propel2? :)

@rozwell

This comment has been minimized.

Show comment
Hide comment
@rozwell

rozwell Aug 22, 2012

Contributor

Sure, but after I fix onDelete="setnull" issue.

Contributor

rozwell commented Aug 22, 2012

Sure, but after I fix onDelete="setnull" issue.

@willdurand willdurand referenced this pull request in propelorm/Propel2 Aug 22, 2012

Closed

Port PR #359 from Propel 1.6 #294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment