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

Honour 'Order Direction' passed into GetPagedResultsByQuery #3692

Conversation

Projects
None yet
5 participants
@marcemarc
Copy link
Contributor

commented Nov 15, 2018

Wanting to update the MiniListView so that you can change the sort order of the results, so that the most recent items are first, eg sort by descending, however chasing this through to the Entity Repository GetPagedResultsByQuery although the sort direction is passed at each stage, inside this method it is ignored... is this the best way to add it back in?

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes:
This is kind of related to this issue, it's a first step: #2957

Description

The change here is to implement OrderByDescending in the GetPagedResultsByQuery in the EntityRepository, it's passed in, but does not appear to be implemented.

One way to test would be to setup a listview, and create a picker, to pick from it (eg invoking the MiniListView)

and tweak the hardcoded order direction

var miniListView = {
node: node,
loading: true,
pagination: {
pageSize: 10,
pageNumber: 1,
filter: '',
orderDirection: "Ascending",
orderBy: "SortOrder",
orderBySystemField: true
}
};

in the MiniListViewDirective to be Descending

switching between the two here (and clearing cache), should order the results displayed in the picker to be ordered in the alternative direction.

(I kind of think the default setting for the MiniListView should be descending - so as an editor, you always see the most recent things first...but that's probably a larger conversation - but for the time being, unless I've totally misunderstood, this is 'worth' implementing, because you'd sort of expect it to work?)

Honour 'Order Direction' passed into GetPagedResultsByQuery
Wanting to update the MiniListView so that you can change the sort order of the results, so that the most recent items are first, eg sort by descending, however chasing this through to the Entity Repository GetPagedResultsByQuery although the sort direction is passed at each stage, inside this method it is ignored... is this the best way to add it back in?
@emmaburstow

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

Oh hi Marc. Thanks for the Pr! We'll look over and let you know if we need anything.

Speak soon,

Em

@poornimanayar

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Hi @marcemarc ,

Am I right in understanding that the PR does not fix the problem in its entirety, but is a start in the direction? The method has been amended to account for the direction but the UI needs to accommodate this as well with changes.

Poornima

@marcemarc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

Hi Poornima

Yes, kind of, this references the original issue, as that's the context that I'm coming from - but this is sort of a separate issue in its own right, eg the GetPagedResultsByQuery accepts a parameter for order direction and then doesn't use it...

... so this might be 'by design' in which case we should remove the Order Direction parameter from the GetPagedResultsByQuery, and update the documentation to avoid confusion...

... if it's not 'by design' though - then this PR fixes this!

What I'm thinking with regard to the original issue, is it isn't worth thinking about the UI for switching order on the MiniListView or whether to try something audacious with linking the picker to the ListView config, if the underlying api method doesn't implement the reverse ordering!

Also I know I can fix the reverse ordering here, but I might not be the best person to look at the original issue...

With the project I'm working on, I'll probably hardcode the MiniListView config to be in reverse order as an experiment for my editors, and then report back... - but I won't do that if there isn't the intention to support sort order through the APIs, hence the PR to start the conversation... eg is there a super good reason not to implement it here?

regards

Marc

@poornimanayar

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Hi @marcemarc ,

Thank you so much for the explanation. What you have mentioned above regd the API is absolutely right. Whether we do or do not change the UI for the Mini ListView the starting point should be the API. After I replied to you earlier today I quickly tested the work and I can confirm that its all working.But I think all your questions/thoughts are valid. So bringing this to the attention of @nul800sebastiaan as well :-)

Regards,
Poornima

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

Could it be that this will lead to problems when you're on page 3 instead of the first page? Haven't looked into this yet, but that's a question that springs to mind. I'll have to discuss at HQ anyway though, will get back to you.

@marcemarc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@nul800sebastiaan This PR is kind of independant of what we may or might do with the MiniListView, that's just the context that has enabled me to discover the issue.... even as a package dev, I'd like to use the Umbraco apis to retrieve and manipulate data, and it's ace you provide an endpoint called 'GetPagedResultsByQuery' and it's ace that this accepts an 'order direction'... what's rubbish is that it doesn't do anything with that 'order direction' :-P

So what I'm asking is... is it an oversight? or a clever 'by design' thing that I don't understand?, fixing the api, will enable package devs, and experimenters to toy with listing orders of items in the context of their custom developments, when using the apis...

eg if it isn't a good idea to make the api 'do what it appears to say it can do' then we can document it, or remove this functionality from it to make it clearer to people trying to use it!

So can we do this fix first? it can then lead onto a debate about the MiniListView under a different PR (it will take time to discuss, I'd guess) but if we fix this thing now, gets me out of a situation on my current project!

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Looks wonderful @marcemarc and this would work just fine. I'll merge!

Ps. Are you going to be looking into the orderBy as well?

@nul800sebastiaan nul800sebastiaan merged commit 150ab13 into umbraco:dev-v7 Dec 19, 2018

1 of 2 checks passed

Cms Continuous #201811150008 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@marcemarc

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

@nul800sebastiaan do you mean for the mini list view? I'm in a slight quandary, eg see the original issue raised #2957 (not by me) - in that in the two scenarios I have editors picking from a ListView, using the mini list view it makes perfect sense for them to be in reverse order by default, but I'm aware that this may not be the case in all scenarios - so I'm thinking a toggle on the order in the mini list view to switch between A-Z and Z-A based on the sort order, but there is the much more ambitious idea of trying to link the picker to the configuration of the ListView it's picking from... that I'm not so sure about.

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@marcemarc what seb is mentioning is the other parameter (orderBy) of the EntityRepository.GetPagedResultsByQuery . What you've fixed is something that has been completely overlooked somehow - the orderDirection parameter wasn't being used which you've fixed but strangely the orderBy parameter isn't used at all either!

This is also still not fixed in v8 so would be good to get this method working properly and with unit tests in the EntityServiceTest class

@Shazwazza

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

@marcemarc I'm actually fixing all of this in v8 but the code there is quite a lot different than the code in v7 so if you pursue the change that's cool but just know that the v8 side of things will be done as far as the EntityRepository goes.

@marcemarc

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

@Shazwazza thanks, yes, was focussing on the direction only! - aim was to solve a specific problem with picking from a ListView for a client, who 'picks' their recent News articles to be included in a Newsletter template, from a News section, segregated into years, each year a separate ListView - when it gets to December they have just over 1000 articles, so super handy to reverse the order direction on my article picker, so they immediately see the 'latest articles' in the mini list view - so selfishly my change above, makes the big improvement they need... whereas changing the field the elements are 'ordered by' (although similarly not implemented) has less impact ... so just pulling this fix in now is really helpful to me and I need to get more feedback from editors once they have used the reversed picker in the new year, to understand what further changes would be helpful, eg toggle the order on the picker.. but I won't let my OCD look at the OrderBy field if it will be revamped in V8!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.