Fixes #11209 - Fix PrefixIPAddress view with saved sort preferences

[DanSheps]

Fixes: #11209 - Do not add available ips when IPAddressTable sort preferences are saved

• Exclude the IPAddress ViewTab from adding available IP annotations when table is sorted.

Checked other views (PrefixPrefixes, PrefixIPRanges) for this and is only present in the IPAddress view (Prefixes are annotated with actual prefixes, IPRanges have no such annotation)

[jeremystretch]

This removes the annotations when an ordering is specified, however it does not seem possible to restore as the ordering is saved in the user's config as ['']. This may require a separate bug report to address.

[jeremystretch]

I've opened #12914 to address the above bug.

[DanSheps]

Do you need anything cleaned up on this one?

[jeremystretch]

Unfortunately I'm not sure this approach is viable. Even with the fix for #12914 in, prep_table_data() is called before the prior ordering is cleared from the UserConfig data. So, if you order the table by IP and then clear the ordering, the "available" annotations aren't added on the second request, but they are added on successive requests because by then the ordering has been cleared.

[DanSheps]

Unfortunately I'm not sure this approach is viable. Even with the fix for #12914 in, prep_table_data() is called before the prior ordering is cleared from the UserConfig data. So, if you order the table by IP and then clear the ordering, the "available" annotations aren't added on the second request, but they are added on successive requests because by then the ordering has been cleared.

TLDR;

1. Order by IP
2. Clear the ordering
3. Revisit the tab

The annotations would not exist on #1 and #2, but would on #3, correct?

The behaviour around #3 and #1 is desired, but the behaviour around #2 is not. I think we would need to clean up the #2 as that should show the annotations if the order has been cleared, but that might simply be also adding a check for "is None" or whatever it gets to when you clear it. I can work on that.

I think it might be worth it to also wrap some tests around this. I think this is how we want to have tests work:

State/Action	Current Behaviour	Desired Behaviour
First visit/default	Annotations visible	Annotations visible
Sort by a column (Ex: name)	Annotations not visible	Annotations not visible
Re-visit tab after sort	Annotations visible	Annotations not visible
Clear column sort	Annotations not visible	Annotations visible
Re-visit tab after clearing sort	Annotations visible	Annotations visible

Could you confirm this? I think it would be trivial to fix the logic and build a test around that specific for IPAddresses

[DanSheps]

I think I have this sorted out properly. Let me know if you want me to try and add in some table tests.

[jeremystretch]

Thanks for your work on on this @DanSheps but IMO we should find a cleaner way to evaluate the table's ordering logic. This might entail refactoring prep_table_data() to pass the table itself.

[DanSheps]

Thanks for your work on on this @DanSheps but IMO we should find a cleaner way to evaluate the table's ordering logic. This might entail refactoring prep_table_data() to pass the table itself.

I think this is a chicken and egg problem. You need to generate the table before you can configure() it which is where the ordering changes happen, but you can't make the ordering changes without first initializing the table (which includes the data as part of initialization).

The only thing I could think of if we refactor would be to call get_table() before with an empty dataset then take the data from get_table_data() and overwrite the data in the table class. Something like:

table = self.get_table(table_data, request, has_bulk_actions)
table_data = self.prep_table_data(request, child_objects, instance, table)
table.data = TableData.from_data(data=table_data)
table.data.set_table(table)

To me, that seems messy and could have unintended consequences, it does do a lot with that data within the init() method.

It may make more sense to have the prep_table_data be part of the table class, but I can't see that working really well either unless we have a different table class for ObjectChildrenView.

[DanSheps]

Tested latest changes. LGTM.