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

[2.3 beta] Publish table no longer retains chosen column sorting #977

Closed
nickdunn opened this issue Dec 18, 2011 · 21 comments
Closed

[2.3 beta] Publish table no longer retains chosen column sorting #977

nickdunn opened this issue Dec 18, 2011 · 21 comments
Assignees
Milestone

Comments

@nickdunn
Copy link
Contributor

I am using the default workspace, viewing the Articles table.

  1. Click the Date heading, Symphony correctly sorts
  2. Use the navigation to get to the Articles page again, and it defaults back to the first column

The main difference I noted is that in previous versions, clicking a column header would load the URL with the ?sort=... parameter, and Symphony would then redirect back to the clean URL once the option had been persisted to the database. Ths redirect no longer occurs, and the choice is only active so long as the sort param is in the URL. Once it is removed, the default sorting is used.

So I don't think this is being persisted to the database any longer.

@nickdunn
Copy link
Contributor Author

Inspecting this further, Symphony is persisting the value to the database. But seems to be getting muddled up somewhere. No idea what changed with this logic so not best placed to begin regression hunting, sorry!

@ghost ghost assigned simoneeconomo Dec 18, 2011
simoneeconomo added a commit to simoneeconomo/symphony-2 that referenced this issue Dec 18, 2011
@simoneeconomo
Copy link
Contributor

@nickdunn: does the following commit work for you? simoneeconomo@60aef93

@brendo
Copy link
Member

brendo commented Dec 19, 2011

Two regressions:

  • Adding ?unsort doesn't reset the sort to the default
  • We now have a query string when the section isn't sorted on the first field

This is the original code for sorting in 2.2.5

@simoneeconomo
Copy link
Contributor

Right, when I updated the sorting logic I must have missed these things, sorry!

May I ask if ?unsort is necessary? There's no real unsorting in the Publish area, it's the same as sorting the first appearing column by ascending order. Concerning the query string, I think it's useful to explicitly see what field is being sorted and how.

@ghost ghost assigned simoneeconomo Dec 19, 2011
@nickdunn
Copy link
Contributor Author

May I ask if ?unsort is necessary? There's no real unsorting in the Publish area, it's the same as sorting the first appearing column by ascending order

The previous behaviour was that unsort would yield the default sort order of System Date, which I think should remain.

Concerning the query string, I think it's useful to explicitly see what field is being sorted and how

I still prefer old behaviour, keeping this out of the querystring. The UI should make this obvious enough. If it doesn't, this is a UI problem (I logged a bug saying the contrast of the selected column was not enough).

I think it should be reverted to 2.2.5 behaviour.

@simoneeconomo
Copy link
Contributor

Fair enough, if that's not urgent I'll personally prepare a pull request by the end of this week.

@brendo
Copy link
Member

brendo commented Dec 19, 2011

Concerning the query string, I think it's useful to explicitly see what field is being sorted and how.

While having a query string gives visibility, it's really not useful to anyone as it uses the field_id instead of the field handle so I think this is a moot point.

There's a couple of considerations for Sortable that we've overlooked. 2.2.x only had sorting for Entries and how it worked was that when a sort was applied, this value was stored in the database as the default sorting column for this section. Visits to the entries table looked up this value and persisted the sort across views.

The ?unsort functionality helped break a deadlock that was caused when a field provided bad SQL (or generally just something went wrong). This would render the entries table useless as upon building the page, the orderable field would be pulled from the database and cause the page to error out. ?unsort removed the sorting for the section, resetting it to null.

I think it should be reverted to 2.2.5 behaviour.

Which would be easy but... With our new approach to sorting, we don't have the equivalent columns in the database to remember a sort value which means that sorts won't persist across views (not pagination, I mean leaving the entries table and then returning to it later). I think this is poor UX, so we'll need to find a solution.

One possible idea is to have a global sorting table that remembers the sort order for all of our sortable table views (Publish, Pages, Datasources, Events and Utilities now). A further improvement could be remembering the sort order on a per author basis.

@nickdunn
Copy link
Contributor Author

Why has sorting drastically changed, and why is there no longer a database column for this? Pretty sure I saw this in the beta, an integer changing in the sections table when I clicked the heading. What changed exactly?

@nickdunn
Copy link
Contributor Author

Reading your comment again, I presume the persisting of column sort was removed when the Ds/Events stuff was introduced, so that all views were the same? In which case I think the Sections functionality exactly as it was in 2.2 should be applied to the new views (clean urls, persistent column), rather than removing it from the existing sections views.

@simoneeconomo
Copy link
Contributor

Why has sorting drastically changed, and why is there no longer a database column for this?

I'm not sure about what @brendo meant with his comment, but the two columns called entry_order and entry_order_direction to store sort options were never removed. Looking at migrations, there doesn't seem to be any update scripts that drops them, so I think there has been a misunderstanding.

If you look at my commit, I'm using those columns to reintroduce the persistence of sorting options across sessions.

brendo added a commit that referenced this issue Feb 15, 2012
@brendo
Copy link
Member

brendo commented Feb 15, 2012

Ok the above commit implements ?unsort and fixes a bug where Fields that shouldn't be sortable (defined by Field->isSortable()) were able to be set as sortable.

The URL's are still 'messy', and as @eKoeS said, making Sections persistent is easy because we have database columns. Datasource and events do not have such columns, and given our long term goal of moving schema into the filesystem (or at least given the option to choose), I don't think this will be possible.

That said, I think we can make use of the config to store these values for Datasources and Events :) New keys, datasource_index_sort/direction and event_index_sort/direction could hold these values and allow persistence.

@simoneeconomo
Copy link
Contributor

That said, I think we can make use of the config to store these values for Datasources and Events :) New keys, datasource_index_sort/direction and event_index_sort/direction could hold these values and allow persistence.

That's nice :)

making Sections persistent is easy because we have database columns.

In a future release it would be nice to move them to the filesystem as well. We have migrations for such a thing. :)

@brendo
Copy link
Member

brendo commented Feb 15, 2012

Exactly :)

@simoneeconomo
Copy link
Contributor

So, if the goal is to switch back to 2.2.x behaviour, I can take care of this by:

  • cleaning URLs (which, as @brendo said, are still "messy")
  • adding new keys in the config file for Datasources/Events sorting
  • moving publish sorting info from the database to the config file (that seems quite easy)

@brendo
Copy link
Member

brendo commented Feb 17, 2012

Yep, thanks Simone :)

@simoneeconomo
Copy link
Contributor

moving publish sorting info from the database to the config file (that seems quite easy)

Question: should these keys be created in the config file upon saving a new section or upon sorting its entries for the first time?

@nickdunn
Copy link
Contributor Author

I'd say sorting for the first time. If they don't exist when rendering the page then the default should be used (ID or date, desc). This keeps the config clean, only storing config if its's needed.

@simoneeconomo
Copy link
Contributor

Makes sense. Next question would be: if sort and order equal to the default values, do we retain existing settings in the config file or do we remove them until custom sorting is applied again?

@nickdunn
Copy link
Contributor Author

The only way to revert to the default is to apply ?unsort to the querystring, which is relatively uncommon at the moment. I'd suggest removing from the config in this instance... only for consistency, no other reason.

@simoneeconomo
Copy link
Contributor

only for consistency, no other reason

Yes, that's what I would suggest too.

@brendo
Copy link
Member

brendo commented Feb 27, 2012

Reopening for a reminder to drop the entry_section columns from tbl_sections and to remove from the default SQL for installation.

@brendo brendo closed this as completed in 796f4cf Jul 31, 2012
brendo added a commit that referenced this issue Jul 31, 2012
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

No branches or pull requests

3 participants