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

Refactoring theme helper #9316

Merged
merged 3 commits into from
Jan 26, 2021
Merged

Conversation

escopecz
Copy link
Sponsor Member

Q A
Branch? 3.0 for bug fixes
Bug fix? yes
New feature? no
Deprecations? yes
BC breaks? no
Automated tests included? yes
Related user documentation PR URL /
Related developer documentation PR URL /
Issue(s) addressed /

Description:

The goal was to enable to set directory name in a separate parameter to the copy() method. As generating it from the theme name may not be always what we want. Also to provide greater test coverage to the TestHelper. However this task was not possible without refactoring.

The ThemeHelper class was not fully testable as it was creating instances of Filesystem and Finder classes instead of providing those helpers via dependency injection so those could be mocked.

So I created 2 new services for those Symfony helpers and even created Mautic's own Filesystem helper by extending Symfony's Filesystem and adding the readFile() method to our implementation so we could use this one helper to manipulate files and folders. Symfony's version doesn't have that method for wrong reasons. Se need an abstraction above the native PHP methods that manipulate files so we could mock outputs in unit tests.

As we now have our own Filesystem helper I have deprecated Symfony's helper.

Steps to test this PR:

  1. Load up this PR
  2. Go to Themes
  3. Clone a theme. Ensure it's visible in the list of Themes as well as in the Email form page.

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Oct 19, 2020
@escopecz escopecz added automated-tests Anything related to unit, functional or e2e tests code-review-needed PR's that require a code review before merging labels Oct 19, 2020
@codecov
Copy link

codecov bot commented Oct 19, 2020

Codecov Report

Merging #9316 (cbe7c89) into features (4ec27ae) will increase coverage by 0.01%.
The diff coverage is 56.09%.

Impacted file tree graph

@@              Coverage Diff               @@
##             features    #9316      +/-   ##
==============================================
+ Coverage       37.66%   37.67%   +0.01%     
- Complexity      33824    33826       +2     
==============================================
  Files            1965     1966       +1     
  Lines          104533   104535       +2     
==============================================
+ Hits            39368    39388      +20     
+ Misses          65165    65147      -18     
Impacted Files Coverage Δ Complexity Δ
app/bundles/CoreBundle/Helper/ThemeHelper.php 59.00% <54.05%> (+9.00%) 78.00 <6.00> (ø)
app/bundles/CoreBundle/Helper/Filesystem.php 75.00% <75.00%> (ø) 2.00 <2.00> (?)

@RCheesley RCheesley added the hacktoberfest Issues that would be great for Hacktoberfest participants to work on label Oct 26, 2020
@escopecz escopecz added the hacktoberfest-accepted PR's that have been accepted for the purposes of Hacktoberfest label Oct 28, 2020
@npracht npracht changed the base branch from staging to features January 19, 2021 16:18
kuzmany
kuzmany previously approved these changes Jan 23, 2021
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.

Tested and works like before.
Code looks good and understand new possibilities
Unit test included.
Then approve it 👍

@kuzmany kuzmany added the pending-test-confirmation PR's that require one test before they can be merged label Jan 23, 2021
@npracht npracht added this to the 3.3.0 milestone Jan 24, 2021
RCheesley
RCheesley previously approved these changes Jan 26, 2021
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Tested on Mautibox and was able to install a new theme cloned from an existing one, and use it to create an email.

@RCheesley RCheesley 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 Jan 26, 2021
@RCheesley RCheesley dismissed stale reviews from kuzmany and themself via 87d0562 January 26, 2021 17:55
@RCheesley RCheesley merged commit 157bf8c into mautic:features Jan 26, 2021
@escopecz escopecz deleted the refactoring-theme-helper branch January 24, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests Anything related to unit, functional or e2e tests cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging hacktoberfest Issues that would be great for Hacktoberfest participants to work on hacktoberfest-accepted PR's that have been accepted for the purposes of Hacktoberfest ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants