FIX: put current page at top of ClassName dropdown (was broken)

sunnysideup commented 2 months ago

Description

For the "Page Type" field in the settings tab (main) of a page, the current Page Type is supposed to be on top (see code changed). The code did not actually do that though and instead just placed the "last" entry in the list on top. That does not make any sense so I fixed it.

Manual testing steps

Open any SS website, go to the settings of a page and look at the list of Page Type options. You will see they are in alphabetical order, but the last one (e.g. the one that starts with last letter in the alphabet is listed first). The current page type is supposed to be listed first!

Issues

https://github.com/silverstripe/silverstripe-cms/issues/3005

Also see: https://github.com/silverstripe/silverstripe-cms/issues/2209 https://github.com/silverstripe/silverstripe-cms/issues/2208

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] This change is covered with tests (or tests aren't necessary for this change)
[x] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green

GuySartorelli commented 2 months ago

The "Issues" section is for you to link to an issue. This is made clear in the pull request template:

List all issues here that this pull request fixes/resolves. If there is no issue already, create a new one! You must link your pull request to at least one issue.

Please create a GitHub issue if there isn't one already and link to it. If there's already an issue, please link to that.

GuySartorelli commented 2 months ago

You've also put "[ N]" next to "This change is covered with tests (or tests aren't necessary for this change)"

Does this mean you think tests are needed, but haven't included them? Please either include tests, or explain why tests aren't needed.

sunnysideup commented 2 months ago

Does this mean you think tests are needed, but haven't included them?
Please either include tests, or explain why tests aren't needed.

Test are nice, but I dont have time for it and I dont think such a simple piece of code needs a test (famous last words), partially because it has not had a test so far, AFAIK.

sunnysideup commented 2 months ago

In Terms of a test, I think it would be more valuable for a human to see that the UX feels good and works nicely. I was thinking that you could have:

-- MyPageType (current type) -- 
OtherPageType1
OtherPageType2

etc... So that you can always see what the current one is.

GuySartorelli commented 2 months ago

The point of unit tests is primarily to prevent regressions. This is a perfect example of something that was working at some point, but due to lack of unit tests it broke at some stage without anyone noticing.

To prevent this breaking again, it would be best to have a unit test.

sunnysideup commented 2 months ago

What exactly would you test for? Would you test that it returns an array or that it returns a valid array? @GuySartorelli - I'd be curious if you had any ideas on what exactly to test for.

sunnysideup commented 2 months ago

I have added a bunch of tests.

michalkleiner commented 2 months ago

Just passing by, left a few minor comments. Thanks for taking the time to add the tests as well, @sunnysideup!

silverstripe / silverstripe-cms