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.

Sign up for GitHub

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

Jump to bottom

Upgrade report bundle to work with Symfony 3 forms #8056

Merged
merged 7 commits into from Dec 13, 2019
Merged

Upgrade report bundle to work with Symfony 3 forms #8056

merged 7 commits into from Dec 13, 2019

Conversation

dongilbert
Copy link
Member

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

Q A
Bug fix? N
New feature? Y
Automated tests included? N
Related user documentation PR URL N/A
Related developer documentation PR URL N/A
Issues addressed (#s or URLs) #7989
BC breaks? N/A
Deprecations? N/A

Description:

Upgrades the Report Bundle to work with Symfony 3 forms.

Steps to test this PR:

  1. Load up this PR
  2. Ensure that creating reports works
  3. Ensure exporting reports work
  4. Ensure that the detail view of a report shows the correct data

@dongilbert dongilbert changed the base branch from staging to m2-to-m3 November 1, 2019 14:04
@npracht npracht added this to the 3.0.0 milestone Nov 1, 2019
@npracht npracht added code-review-needed PR's that require a code review before merging Mautic 3 ready-to-test PR's that are ready to test labels Nov 1, 2019
npracht
npracht requested changes Nov 1, 2019
View reviewed changes
Copy link
Member

@npracht npracht left a comment •
edited

Choose a reason for hiding this comment

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

I tested and it works

  • publish/unpublish
  • opening native reports (already created reports on fresh instance)

Not working

  1. Try to view native report n°3 (https://mautibox.com/8056/s/reports/view/3)
    Capture d’écran 2019-11-01 à 16 36 23
  2. Click on "New" report button
    Capture d’écran 2019-11-01 à 16 36 59
  3. Try edit any native report
    Capture d’écran 2019-11-01 à 16 38 09

@npracht npracht added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Nov 1, 2019
@npracht npracht added pending-test-confirmation PR's that require one test before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author and removed pending-feedback PR's and issues that are awaiting feedback from the author pending-test-confirmation PR's that require one test before they can be merged labels Nov 1, 2019
@dongilbert
Copy link
Member Author

These issues have been addressed @npracht

@npracht npracht added ready-to-test PR's that are ready to test and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Nov 1, 2019
npracht
npracht previously requested changes Nov 3, 2019
View reviewed changes
Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

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

I tested and it works

  • publish/unpublish
  • opening native reports (already created reports on fresh instance)
  • Click on "New" report button
  • Try edit any native report

Not working:

[2019-11-03 08:30:50] mautic.ERROR: Symfony\Component\Debug\Exception\ContextErrorException: PHP Notice - Undefined index: count_query - in file /var/app/current/code/pulls/8056/app/bundles/ReportBundle/Views/Report/details.html.php - at line 175

@npracht npracht added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Nov 3, 2019
@dongilbert
Copy link
Member Author

As we discussed in Amsterdam, both errors are present in the branch outside of any changes that I've made - it's an issue with running in Dev mode that's not dependent upon any changes here.

@dongilbert dongilbert dismissed npracht’s stale review November 26, 2019 14:34

Issues presented are present in dev mode without any of these changes implemented. So they are unrelated to this PR.

@dongilbert dongilbert added ready-to-test PR's that are ready to test and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Nov 26, 2019
anton-vlasenko
anton-vlasenko suggested changes Dec 6, 2019
View reviewed changes
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment •
edited

Choose a reason for hiding this comment

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

Please flip values and keys for ChoiceType form types and add choices_as_values option.
https://symfony.com/doc/2.7/reference/forms/types/choice.html#choices-as-values

escopecz
escopecz approved these changes Dec 13, 2019
View reviewed changes
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.

I don't see any issue now. 👍

@escopecz escopecz removed code-review-needed PR's that require a code review before merging ready-to-test PR's that are ready to test labels Dec 13, 2019
@dongilbert dongilbert merged commit 5d02f48 into mautic:m2-to-m3 Dec 13, 2019
@escopecz escopecz mentioned this pull request Dec 13, 2019
Merged
@escopecz
Copy link
Sponsor Member

Failed tests fixed in #8232

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anton-vlasenko anton-vlasenko anton-vlasenko requested changes

@escopecz escopecz escopecz approved these changes

@npracht npracht Awaiting requested review from npracht

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0-beta
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dongilbert @escopecz @npracht @anton-vlasenko