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

Allow array values in api filter paramaters #7933

Merged
merged 4 commits into from
Feb 13, 2021

Conversation

galvani
Copy link
Contributor

@galvani galvani commented Oct 9, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? N
New feature? Y
Automated tests included? Y
BC breaks? N
Deprecations? N

Description:

We do not allow for API calls to use multiple arguments for anything but ids. The goal of this PR is to allow them.
Steps to test and reproduce this PR:

Make simillar API calls with multiple arguments for something else than ids

/api/contacts?where[0][col]=global_customer_id&where[0][expr]=in&where[0][val]="10","11"
/api/contacts?where[0][col]=global_customer_id&where[0][expr]=in&where[0][val]=10,11

They don't work
Load up the PR
They should work correctly now

@npracht npracht added API Anything related to the API enhancement Any improvement to an existing feature or functionality T2 Medium difficulty to fix (issue) or test (PR) ready-to-test PR's that are ready to test labels Oct 9, 2019
@npracht npracht added Triage M2/M3 and removed ready-to-test PR's that are ready to test labels Apr 4, 2020
@npracht
Copy link
Member

npracht commented Apr 4, 2020

Hi there!
It has been decided to not create any extra Mautic 2 minor versions (which means no more features in M2) knowing that Mautic 3 is coming very soon.

We now want to integrate your contribution in the Mautic 3 roadmap as 3.1.0 candidate.

How to do?

  1. Check if your feature or enhancement is still missing and relevant in Mautic 3
  2. If Yes: rebase your PR against the 3.x branch
  3. If No: please consider closing your PR if you are absolutely sure that the feature has made it into Mautic 3 already

Please report results by commenting on your PR to make us administration easier.


You can more information on how to do all of that on this blog post "Getting you PR ready for Mautic 3".

@RCheesley RCheesley added this to the 3.1.0 milestone Jul 22, 2020
@dennisameling
Copy link
Member

@galvani This PR is still based on Mautic 2. Could you please rebase it against the staging branch for Mautic 3 and to trigger a code coverage report? Thanks!

@dennisameling dennisameling added needs-rebase PR's that need to be rebased pending-feedback PR's and issues that are awaiting feedback from the author labels Aug 14, 2020
@RCheesley
Copy link
Sponsor Member

Bumping to 3.2 as this PR requires updating for 3.x and rebasing. cc @escopecz @alanhartless

@RCheesley RCheesley modified the milestones: 3.1.0, 3.2 Aug 16, 2020
@RCheesley
Copy link
Sponsor Member

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Aug 25, 2020

We require contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact @RCheesley. CLA has not been signed by @galvani.

@cla-bot
Copy link

cla-bot bot commented Aug 25, 2020

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

@RCheesley
Copy link
Sponsor Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Sep 30, 2020
@cla-bot
Copy link

cla-bot bot commented Sep 30, 2020

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

@npracht npracht modified the milestones: 3.2.0, 3.2.1, 3.3 Nov 23, 2020
@npracht npracht changed the base branch from staging to features December 1, 2020 07:57
@npracht
Copy link
Member

npracht commented Dec 1, 2020

@mautic/acquia-po is it still actual and relevant for M3 ?

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #7933 (2bd4a24) into features (77f0da0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             features    #7933   +/-   ##
===========================================
  Coverage       38.33%   38.34%           
- Complexity      33939    33941    +2     
===========================================
  Files            1982     1982           
  Lines          104948   104952    +4     
===========================================
+ Hits            40236    40246   +10     
+ Misses          64712    64706    -6     
Impacted Files Coverage Δ Complexity Δ
app/bundles/CoreBundle/Entity/CommonRepository.php 63.56% <100.00%> (+0.98%) 322.00 <0.00> (+2.00)

@escopecz
Copy link
Sponsor Member

escopecz commented Dec 1, 2020

@npracht yes, it is.

@escopecz escopecz force-pushed the staging.api_accept_array_values branch from 56f2897 to 85d83ae Compare December 9, 2020 18:53
@escopecz escopecz removed needs-rebase PR's that need to be rebased pending-feedback PR's and issues that are awaiting feedback from the author labels Dec 9, 2020
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me 👍

@escopecz escopecz added the pending-test-confirmation PR's that require one test before they can be merged label Dec 14, 2020
@RCheesley
Copy link
Sponsor Member

@escopecz any chance you could offer a query to use with sample data to test this?

@escopecz
Copy link
Sponsor Member

escopecz commented Dec 18, 2020

Here is CURL. Paste it to a command line but update the values in capital letters:

curl "https://URL/index_dev.php/api/contacts?where%5B0%5D%5Bcol%5D=firstname&where%5B0%5D%5Bexpr%5D=in&where%5B0%5D%5Bval%5D=John,Jane" \
     -u 'USERNAME:PASSWORD'

In order to get some contacts in the response you have to have some contacts with first name of John and/or Jane

@RCheesley RCheesley self-requested a review February 12, 2021 22:49
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Works for me
Unit test included

Then approve it 👍

@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 13, 2021
@RCheesley RCheesley merged commit bf846ae into mautic:features Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Anything related to the API cla-signed The PR contributors have signed the contributors agreement enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T2 Medium difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants