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

Implement Thank with Google PHP module foundation #5364

Closed
felixarntz opened this issue Jun 15, 2022 · 6 comments
Closed

Implement Thank with Google PHP module foundation #5364

felixarntz opened this issue Jun 15, 2022 · 6 comments
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority PHP Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Jun 15, 2022

We should implement the PHP module foundation for Thank with Google, for a new thank-with-google module. We do currently have a subscribe-with-google module, which however is now indefinitely on hold, and the Thank with Google module has taken over as a priority. Furthermore, the foundation requirements for the two modules are quite similar.

Therefore, we should implement the foundation for Thank with Google by essentially tweaking the Subscribe with Google foundation to become Thank with Google.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The Subscribe_With_Google class and any related names (also the related Settings class namespace) should be renamed to Thank_With_Google.
    • The currently available settings should remain untouched as part of this issue, i.e. we should not tweak the setting keys available, just rename everything on the PHP side to match "Thank with Google".
  • The module slug subscribe-with-google should be consistently changed to thank-with-google in PHP, but not in JS, which is part of the separate Implement modules/thank-with-google JS store foundation #5365 issue.
    • The file name of the JS asset for the module should also not be changed.
  • The module name "Subscribe with Google" should also be changed accordingly to "Thank with Google".
  • Any references to the subscribe-with-google module slug in PHP should be changed to reference the thank-with-google module slug (e.g. also the WordPress option name).
  • The @since annotations for these classes, methods, etc. should be changed to n.e.x.t, since they are basically new classes (even though we technically changed them from the Subscribe with Google classes.
  • The feature flag swgModule should be changed to be called twgModule.

Additional acceptance criteria (update)

  • The new module description should be: Let your supporters show appreciation of your work through virtual stickers and personal messages

Implementation Brief

Note: The ACs are very close to code here already, so no need to rehash all of this. Please review the ACs closely together with the relevant code and point out any more complex changes necessary or follow-up questions.

  • Rename The following files and classes in accordance with the AC.
    • includes/Modules/Subscribe_With_Google.php
    • includes/Modules/Subscribe_With_Google/Settings.php
  • Update the usage in the following classes:
    • Google\Site_Kit\Core\Authentication\Authentication
    • Google\Site_Kit\Core\Modules\Modules
  • Change swgModule feature flag to twgModule in the feature-flags.json.
    • Also change all swgModule in the PHP files.
  • Finish and merge Thank with Google PHP Foundation #5391
    • Fix failing tests.

Test Coverage

  • Update the PHP Test file names and classes to to be Thank_With_Google.

QA Brief

  • In the tester plugin, the swgModule feature flag should no longer exist.
  • Instead, there will be twgModule feature flag.
  • Enabling the feature flag will bring up Thank with Google module in the Settings > Connect more services screen.
  • Clicking connect will not do anything and throw an error, this is expected.
  • Update: The Module description in the Settings Page should match the description from updated AC.

Changelog entry

  • Add foundation for new Thank with Google feature (PHP).
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature labels Jun 15, 2022
@felixarntz felixarntz self-assigned this Jun 15, 2022
@felixarntz felixarntz added the Module: Thank with Google Thank with Google module related issues label Jun 15, 2022
@felixarntz felixarntz removed their assignment Jun 15, 2022
@kuasha420 kuasha420 self-assigned this Jun 16, 2022
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Jun 18, 2022
@eugene-manuilov eugene-manuilov self-assigned this Jun 21, 2022
@eugene-manuilov eugene-manuilov added PHP Good First Issue Good first issue for new engineers labels Jun 21, 2022
@eugene-manuilov
Copy link
Collaborator

@kuasha420, please, do not use "PR as IB" approach for regular tickets, we strongly discourage this approach even if a ticket seems pretty straightforward.

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Jun 21, 2022
@kuasha420
Copy link
Contributor

@eugene-manuilov Great point. I didn't really intend to include the PR in the IB and I was just going through the AC to see if there's anything additional that needs to be done. Added the draft PR as I already had done the initial changes and thought it could be a starting point. This one and #5365 was tricky as the AC was basically a high level IB.

I will keep that in mind for future! Thanks 👍

@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Jun 21, 2022
@techanvil techanvil assigned techanvil and kuasha420 and unassigned techanvil Jun 22, 2022
@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Jun 23, 2022
@techanvil techanvil removed their assignment Jun 24, 2022
@mohitwp mohitwp self-assigned this Jun 24, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 24, 2022

QA Update ✅

Verified

  • The swgModule feature flag is removed.
  • The twgModule feature flag is added.
  • Enabling the feature flag bring up Thank with Google module in the Settings > Connect more services screen.

image

image

@mohitwp mohitwp removed their assignment Jun 24, 2022
@felixarntz
Copy link
Member Author

@kuasha420 This is basically good to go, I just have one additional request that should be a super quick thing to add in a new PR: Apologies, I missed replacing the module description in the original ACs, which I've added to the ACs now. Could you open another PR (against main if it 1.78.0 is already branched off to it) to replace this (basically just change https://github.com/google/site-kit-wp/pull/5391/files#diff-f484944f8f737a196b627b845da4d99e5ebc0959adb9eb27b21fdb695b3e079eR108)?

@kuasha420
Copy link
Contributor

@felixarntz Done. I've assigned it back to you. Do we need to go through QA again, I have added a QAB just in case but feel free to decide on it after merging. Cheers. (Also I don't think main is cut yet so PR'd against develop).

@felixarntz
Copy link
Member Author

@kuasha420 Excellent, thanks for the quick PR! No need to go through QA again since this is literally just the single text string change :)

@felixarntz felixarntz removed their assignment Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority PHP Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants