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

FIX Allow convertSearchToArray to handle repeating elements #1649

Open
wants to merge 1 commit into
base: 1.13
Choose a base branch
from

Conversation

mikenuguid
Copy link
Contributor

@mikenuguid mikenuguid commented Jan 12, 2024

Description

Manual testing steps

Issues

Pull request checklist

  • The target branch is correct
  • 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.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

- passing a query string containing identical key with distinct values should be parsed and stored correctly
@mikenuguid mikenuguid marked this pull request as ready for review January 12, 2024 03:42
@GuySartorelli
Copy link
Member

Hi,

Thank you for the contribution.

I see that you've deleted the PR templte. I've added it back in. Can you please fill in the template, and check all of the boxes that apply?
This really helps us get a feel for what state your contribution is in, what it does, and gives us a good starting point to review it.

@mikenuguid
Copy link
Contributor Author

@GuySartorelli any feedback on this? There's a merge conflict which needs to be resolved too.

@GuySartorelli
Copy link
Member

Sorry, I hadn't noticed you had updated the PR description.
Can you please add some manual testing steps? It's not clear how I can test this piece of work.
Please reply when you've updated it so I know it's ready for me to look at it again.

@mikenuguid
Copy link
Contributor Author

@GuySartorelli you could use content review report.

  • Once added, open one of the existing reports e.g. pages due for review report
  • Update paremeterFields() method with a ListboxField e.g.
$filtersList->push(
    ListboxField::create('testlistbox', 'listbox', ['1', '2', '3', '4'])
);
  • Open the updated report on the CMS and select at least 2 options from the listbox
  • The URL should include both options on the filter e.g.
admin/reports/show/SilverStripe-ContentReview-Reports-PagesDueForReviewReport?filters%5Btestlistbox%5D%5B%5D=0%2C1 // admin/reports/show/SilverStripe-ContentReview-Reports-PagesDueForReviewReport?filters[testlistbox][]=0,1
  • Previously it only displays a single value, now it should display multiple values

@GuySartorelli
Copy link
Member

Following those instructions, both before and after the PR I get
?filters%5Btestlistbox%5D%5B%5D=0&filters%5Btestlistbox%5D%5B%5D=1 (i.e. ?filters[testlistbox][]=0&filters[testlistbox][]=1) as the query string - which is correct.

It seems like this is making no change for me. Tested with both firefox and chromium.

Are there any more steps you can provide me to reproduce this from a fresh installation (with or without silverstripe/contentreview added)?

@mikenuguid
Copy link
Contributor Author

That's different from what I'm getting. Also, the output of that js method (convertSearchToArray) should not contain duplicate keys so the GET params should be filters[testlistbox][]=0,1 (if fix is applied) and filters[testlistbox][]=1 (if not).

@GuySartorelli
Copy link
Member

That's different from what I'm getting.

Okay, well... if you can give me sufficiently detailed steps to reproduce both the original problem and the behaviour you're seeing with your PR I'll be happy to review further - but if I can't reproduce the bug nor the results you're seeing with the PR I can't merge this.

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.

2 participants