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 a management command which re-orders a OrderModel #188

Conversation

flupzor
Copy link
Contributor

@flupzor flupzor commented Mar 4, 2019

Very often I end up in the case where the 'order' values are no longer unique. Usually it's because of one of the fields in 'order_with_respect' to is changed, and the code does not deal well with this situation. This management command repairs the ordering, so that it can become usable again.

@imomaliev
Copy link
Collaborator

duplicate #174

@flupzor
Copy link
Contributor Author

flupzor commented Mar 5, 2019

Ah, I didn't notice due to the PR title being very non-descriptive. Looks like my PR does more than the other PR and does everything which is requested there.

The only thing I see is that line 53-56 of my PR might have to be removed. This, probably, skips over the mentioned bug in the ticket.

        # Skip if none of the orderings overlap.
        order_values = list(queryset.values_list(order_field_name, flat=True))
        if len(set(order_values)) == len(order_values):
            return

I can also add a test which verifies that if gaps are introduced they are re-numbered properly. I was under the wrong assumption that django-ordered-model automatically fixes these gaps. That's why I didn't test for this.

@shuckc
Copy link
Member

shuckc commented Mar 5, 2019

Yes this looks much better. Worth comparing the tests etc. After renunbering there should be no gaps in the numbering for each ordered group.

…me reason the ordering was corrupted.

In certain cases the models will end up in a not properly ordered state. This can be caused
by bypassing the 'delete' / 'save' methods, or when a user changes a foreign key of a object
which is part of the 'order_with_respect_to' fields.

This management command allows you to repair this invalid state.
@flupzor
Copy link
Contributor Author

flupzor commented Mar 11, 2019

I've included what I thought was useful from the other PR. The test didn't actually work, I modified the test to do what I think the original intent was.

I also changed the description in the README somewhat, because, I don't think it is true that a delete does not re-order in the admin (Haven't verified this). Only bulk deletes would not work (Does the admin even do bulk deletes?), or if the delete method is somehow bypassed.

I also removed the 'start_number' argument, because I don't see how that is useful.

@ronaldgrn
Copy link

When can we get this merged? Or is there an alternative solution for this problem?

@shuckc shuckc mentioned this pull request Apr 8, 2021
@shuckc
Copy link
Member

shuckc commented Apr 8, 2021

I've rebased this, dropped six for python 2 support, fixed up black formatting, modified the tests not to write to the command line during test runs, added assertions for management command cli output and verified tox runs clean locally. It looks like when the PR was opened @flupzor has not allowed contributors write access, so I have pushed this to a branch reorder-management-command - we might need to open a new PR to merge it.

Certainly there is demand for it - see #236 #174 #64 and since the code is disjoint from what we have there's no risk of regressions, so I would like to get it merged for the next release.

@shuckc
Copy link
Member

shuckc commented Apr 14, 2021

Closing as taken forward on #240

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.

None yet

4 participants