Skip to content

Add back FB extension to Installed marketing extensions list. #34259

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
Merged
findingsimple merged 3 commits into from
Aug 26, 2022
Merged

Add back FB extension to Installed marketing extensions list. #34259

findingsimple merged 3 commits into from
Aug 26, 2022

Conversation

@rawdreeg
Copy link
Contributor

@rawdreeg rawdreeg commented Aug 10, 2022 •
edited by ecgan
Loading

All Submissions:

Changes proposed in this Pull Request:

We've received a request (see pbIJXs-2mT-p2#comment-4795) to restore Facebook for Woocommerce to the Installed marketing extensions list. This was previously removed here: #33781

How to test the changes in this Pull Request:

  1. Check out this branch.
  2. then navigate the Marketing in your WooCommerce site - example.site/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing
  3. Confirm that FB is listed in the "Installed marketing extensions" section of the marketing page.

Before this change:

image

After this change:
Screenshot 2022-08-11 at 15 03 46

Other information:

  • [ x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable? - N/A
  • [ x] Have you successfully run tests with your changes locally?
  • [ x] Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Aug 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2022 •
edited
Loading

Test Results Summary

Commit SHA: e256963

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests000000NaNm NaNs
E2E Tests186001018713m 16s
 To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@rawdreeg rawdreeg marked this pull request as ready for review August 11, 2022 13:07
@rawdreeg rawdreeg requested a review from a team August 11, 2022 13:10
@puntope
Copy link
Contributor

puntope commented Aug 12, 2022

❓ Some of the check are failing. It's worth it to investigate if it's related to this PR

puntope
puntope reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

✅ LGTM

Thanks for the PR

I tested locally. After installing fb, it appears in the installed extensions

Screenshot 2022-08-12 at 16 48 23

❓ I left a comment regarding a failing test. Would be nice to check if its related to this PR. Maybe a rebase?

@rawdreeg rawdreeg force-pushed the tweak/re-add-fb-marketing-extensions-list branch from 77d2edb to e256963 Compare August 13, 2022 13:46
@rawdreeg
Copy link
Contributor Author

rawdreeg commented Aug 15, 2022

❓ I left a comment regarding a failing test. Would be nice to check if it's related to this PR. Maybe a rebase?

Thanks, @puntope. I have tried rebasing, but this did not fix the issue. It looks like other PRs are also failing. Eg: #34317.

@puntope puntope self-requested a review August 15, 2022 09:32
@rawdreeg rawdreeg requested a review from findingsimple August 16, 2022 16:12
@findingsimple findingsimple requested review from a team and jorgeatorres and removed request for a team August 23, 2022 07:43
@findingsimple
Copy link
Contributor

findingsimple commented Aug 23, 2022

@peterfabian @jorgeatorres would you mind confirming if it ok for us to merge this? (just want to make sure we are following correct etiquette 😄 )

jorgeatorres
jorgeatorres approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jorgeatorres
Copy link
Member

jorgeatorres commented Aug 23, 2022

would you mind confirming if it ok for us to merge this? (just want to make sure we are following correct etiquette 😄 )

Hi @jconroy! 👋
Now that is has been approved by someone from Proton it should be good to merge. It is still waiting for your review, so I didn't go ahead and merge. In any case, feel free to do that now.

@rawdreeg
Copy link
Contributor Author

rawdreeg commented Aug 24, 2022

Thanks, @jorgeatorres,

@jconroy could you please merge for me? It seems I don't have the authorization to merge. Thanks :-)

@findingsimple findingsimple merged commit 0b772e3 into trunk Aug 26, 2022
@findingsimple findingsimple deleted the tweak/re-add-fb-marketing-extensions-list branch August 26, 2022 10:06
@github-actions github-actions bot added this to the 7.0.0 milestone Aug 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2022 •
edited by rawdreeg
Loading

Hi @jconroy, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@rawdreeg rawdreeg added the release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] label Aug 26, 2022
@ecgan
Copy link
Contributor

ecgan commented Aug 26, 2022 •
edited
Loading

@rawdreeg , I just noticed in the repo that we now have two Facebook icons:

I'm thinking we should just removed the old one, and have the new one named facebook.svg. I can work on changing this, together with a bit more icon enhancement (make it look nicer with the gray background, similar to PR #34471). Just want to get a confirmation from you if we really need to keep two copies of Facebook icons.

@rawdreeg
Copy link
Contributor Author

rawdreeg commented Aug 26, 2022

@ecgan i added the icon because i didn't want to affect existing usage. I agree we should only have one facebook.svg.

I can work on changing this, together with a bit more icon enhancement (make it look nicer with the gray background, similar to PR #34471)

That sounds good. Thanks for looking at this.

@ecgan ecgan mentioned this pull request Sep 6, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorgeatorres jorgeatorres jorgeatorres approved these changes

@findingsimple findingsimple Awaiting requested review from findingsimple

+1 more reviewer

@puntope puntope puntope approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin. release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto]

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@rawdreeg @puntope @findingsimple @jorgeatorres @ecgan