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

sms_pending in search bar #9882

Open
wants to merge 15 commits into
base: 4.0
Choose a base branch
from
Open

sms_pending in search bar #9882

wants to merge 15 commits into from

Conversation

nfakhour
Copy link
Contributor

@nfakhour nfakhour commented Apr 6, 2021

Q A
Branch? "features" for all features, enhancements and bug fixes (until 3.3.0 is released)
Bug fix? yes
New feature? no
Deprecations? no
BC breaks? no
Automated tests included? no
Issue(s) addressed Fixes #9697

Description:

Steps to test this PR:

  1. Load up this PR
  2. go to /s/sms, if you have pending text messages, Click on X pending

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Apr 6, 2021
@codecov
Copy link

codecov bot commented Apr 6, 2021

Codecov Report

Merging #9882 (dfa33a3) into features (4df7500) will decrease coverage by 0.00%.
The diff coverage is 85.33%.

❗ Current head dfa33a3 differs from pull request most recent head 974bd7a. Consider uploading reports for the commit 974bd7a to get more accurate results
Impacted file tree graph

@@              Coverage Diff               @@
##             features    #9882      +/-   ##
==============================================
- Coverage       41.56%   41.56%   -0.01%     
- Complexity      34581    34588       +7     
==============================================
  Files            2063     2063              
  Lines          111574   111640      +66     
==============================================
+ Hits            46378    46402      +24     
- Misses          65196    65238      +42     
Impacted Files Coverage Δ
app/bundles/LeadBundle/Entity/LeadRepository.php 52.89% <ø> (ø)
...dles/LeadBundle/EventListener/SearchSubscriber.php 56.45% <56.52%> (+<0.01%) ⬆️
app/bundles/SmsBundle/Entity/SmsRepository.php 33.77% <98.07%> (+33.77%) ⬆️
...ilBundle/EventListener/MatchFilterForLeadTrait.php 44.44% <0.00%> (-8.06%) ⬇️
app/bundles/LeadBundle/Model/CompanyModel.php 41.52% <0.00%> (-7.50%) ⬇️
app/bundles/CoreBundle/Helper/InputHelper.php 88.58% <0.00%> (-0.55%) ⬇️
...les/EmailBundle/EventListener/ReportSubscriber.php 33.81% <0.00%> (-0.39%) ⬇️
...bundles/PageBundle/Controller/PublicController.php 41.94% <0.00%> (-0.16%) ⬇️
app/bundles/LeadBundle/Model/ImportModel.php 58.60% <0.00%> (-0.16%) ⬇️
...bundles/LeadBundle/Controller/ImportController.php 0.00% <0.00%> (ø)
... and 3 more

@escopecz
Copy link
Sponsor Member

@nfakhour can you please clarify if this is a new feature or a bug fix? It doesn't seem like the sms_pending search filter ever existed:
Screen Shot 2021-04-16 at 12 45 13

What should search sms_pending:ID even do? Should it list all pending SMS messages or message with the ID after the colon?

Also, all new changes have to be covered with unit or functional tests. Please add them. See https://contribute.mautic.org/contributing-to-mautic/developer/code/pull-requests#writing-tests

@escopecz escopecz added needs-automated-tests PR's that need automated tests before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author labels Apr 16, 2021
@nfakhour
Copy link
Contributor Author

nfakhour commented Apr 28, 2021

Sorry @escopecz, I couldn't answer earlier ,
It's a bug fix , when you have pending text messages you see this on the listing , so we have the number of pending messages and the number of sent messages and on the right it's the ID. Capture d’écran 2021-02-16 à 15 16 56
but when you click on 1 pending, it writes in the search bar mautic.lead.lead.searchcommand.sms_pending:12 but instead of showing the leads who are awaiting the text message, there are no results.
Capture d’écran 2021-02-16 à 15 16 39
I hope it's clearer now.

@RCheesley
Copy link
Sponsor Member

@nfakhour thanks for making the PR and for the explanation. Would you be able to rebase on the features branch please, so that it is up to date with the latest version we are working on?

Also, are you able to address the test coverage as requested above?

@RCheesley RCheesley added sms Anything related to SMS T1 Low difficulty to fix (issue) or test (PR) user-interface Anything related to appearance, layout, and interactivity and removed pending-feedback PR's and issues that are awaiting feedback from the author labels May 16, 2021
@nfakhour nfakhour changed the base branch from 3.3 to features May 16, 2021 17:00
@RCheesley RCheesley added the bug Issues or PR's relating to bugs label Sep 7, 2021
@RCheesley RCheesley changed the base branch from features to 4.0 September 7, 2021 14:18
@RCheesley RCheesley added the needs-rebase PR's that need to be rebased label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement needs-automated-tests PR's that need automated tests before they can be merged needs-rebase PR's that need to be rebased sms Anything related to SMS T1 Low difficulty to fix (issue) or test (PR) user-interface Anything related to appearance, layout, and interactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sms_pending:ID in search bar doesn’t work
3 participants