Jetpack Extensions: Use different keys to avoid conflicts between WP and JP extensions by hassaanelgarem · Pull Request #19665 · wordpress-mobile/WordPress-iOS

hassaanelgarem · 2022-11-25T00:12:35Z

Fixes #19591

Description

This PR fixes an issue where jetpack extensions were conflicting with WP extensions. This is done by using different keys for the WP and JP extensions.

P.S: This change means we will have to migrate the extensions' data since they are no longer shared between WP and JP. This will be done in a coming PR and will follow the same pattern as the one in #19663, that's why this PR depends on #19663's branch.

Testing Instructions

Install & run Jetpack
Sign in using account A
Install & run WordPress
Sign in using account B
Go to photos
Select a photo
Tap on the share button
Select Jetpack
Make sure that the site list displayed is the one from account A, not account B.

Regression Notes

Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…for each service

peril-wordpress-mobile · 2022-11-25T00:12:40Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

wpmobilebot · 2022-11-25T01:26:10Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19665-ca11d56 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-11-25T01:28:59Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19665-ca11d56 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

dvdchr

Hi @hassaanelgarem 👋🏼 . Thank you for working on this! I left one non-blocking suggestion, and the changes are looking good to me.

This change means we will have to migrate the extensions' data since they are no longer shared between WP and JP. This will be done in a coming PR [...]

Please note that this PR and the changes you've planned in the next PR need to be all-or-nothing; If we merge this, we must also merge the addition to the migration process.

My recommendation is to either put everything in this PR or don't merge this PR until both the PRs are green. I can't help but think about the risks due to the very tight time constraints 😅

What do you think?

dvdchr · 2022-11-25T06:53:54Z

+
+import Foundation
+
+@objc extension AppConfiguration {


As we will be having a couple of "similar-but-different" configuration files, (e.g., WidgetConfiguration, AppConfiguration, AppConstants), I think it would help to add some documentation/warning here (and in the counterpart file) stating that the keys must be in-sync with the counterpart file.

While this doesn't entirely prevent mistakes, I feel it still helps to deter human errors. More detail on this: #19479 (comment)

Thanks for linking this comment thread 👍.

Yes, we went for this solution without too many discussions, simply following the pattern that was already established with AppConfiguration files. However, it does have drawbacks with discoverability and the necessity to build both targets to ensure there're no errors.

Good point @dvdchr!
I've added documentation here and in the other configuration files c9a58f2 👍

dvdchr · 2022-11-25T06:59:56Z

+
+import Foundation
+
+@objc extension AppConfiguration {


(see the comment above)

momo-ozawa

Works as described. Will we be able to target 21.3, given the code freeze is coming up very soon?

staskus · 2022-11-25T16:03:22Z

    var wordPressNotificationsEnabled: Bool {
        get {
            guard let userDefaults = userDefaults,
-                  userDefaults.value(forKey: WPNotificationsEnabledKey) != nil else {


There'll be conflicts with these after my latest merge.

WPNotificationsEnabledKey and subsequently AppConfiguration.Extension.NotificationsService.enabledKey are no longer needed and could be deleted.

Thanks for the heads up @staskus 🙇

# Conflicts: # WordPress/Classes/Services/NotificationFilteringService.swift # WordPress/Classes/System/Constants.m

…ns-copy-data # Conflicts: # WordPress/Jetpack/Classes/Utility/DataMigrator.swift

hassaanelgarem · 2022-11-27T15:47:50Z

Please note that this PR and the changes you've planned in the next PR need to be all-or-nothing; If we merge this, we must also merge the addition to the migration process.

@dvdchr Agreed. I've added a "Do not merge" label, and I'll remove it once #19666 is ready.

@mokagio FYI. ~~Also, given the time, I don't think #19666 will be ready and approved before the code freeze. Do you think we can include these two PRs in a beta release?~~
Edit: #19666 is merged into this PR, and I've enabled auto-merge on this PR, so it will be merged once the CI passes 👍

…Reader`

dvdchr

Just noticed a typo 😅 But it's looking good overall.

…copy-data Jetpack Focus: Copy Extensions' data to JP

hassaanelgarem added 7 commits November 25, 2022 00:45

Add: Extension configuration files

81c89c1

Change: Update the share extension service to use new constants

4130e6b

Change: Update the notifications service extension to use new constants

31ea4cb

Fix: build issues due to missing target memberships

c10a498

Add: copy share extension data to JP

4e9ca98

Add: copy notifications extension data to the JP app

63d826f

Refactor: create a helper for copying user default keys and reuse it …

70f224d

…for each service

hassaanelgarem added [Type] Bug Share Extension labels Nov 25, 2022

hassaanelgarem added this to the 21.3 milestone Nov 25, 2022

hassaanelgarem requested review from dvdchr and momo-ozawa November 25, 2022 00:12

hassaanelgarem self-assigned this Nov 25, 2022

dvdchr reviewed Nov 25, 2022

View reviewed changes

momo-ozawa approved these changes Nov 25, 2022

View reviewed changes

Base automatically changed from feature/19657-migrate-today-widget to trunk November 25, 2022 12:13

staskus reviewed Nov 25, 2022

View reviewed changes

staskus approved these changes Nov 25, 2022

View reviewed changes

dvdchr approved these changes Nov 25, 2022

View reviewed changes

hassaanelgarem added 4 commits November 27, 2022 17:09

Merge branch 'trunk' into fix/19591-extensions-diff-keys

561bda3

# Conflicts: # WordPress/Classes/Services/NotificationFilteringService.swift # WordPress/Classes/System/Constants.m

Remove: unneeded enabled key constant

6386791

Add: documentation warnings for duplicated configuration files

c9a58f2

Merge branch 'fix/19591-extensions-diff-keys' into fix/19591-extensio…

0030cb5

…ns-copy-data # Conflicts: # WordPress/Jetpack/Classes/Utility/DataMigrator.swift

hassaanelgarem added the [Status] DO NOT MERGE label Nov 27, 2022

hassaanelgarem added 11 commits November 27, 2022 17:48

Add: release note

0b41f24

Update: stop copying deprecated defaults key

28c401b

Add: tests for extensions' data migration

12053b2

Update: add dictionaryRepresentation() to `UserPersistentRepository…

4b3736f

…Reader`

Update: change DataMigrator to use UserPersistentRepository

f3e4981

Update: inject in-memory user defaults in data migrator tests

98244d7

Update: replace use of standard UserDefaults with the injected one

61d91f5

Add: implement test for copying widgets user defaults data

1f7d1c2

Add: implement test for copying share extension user defaults data

b55bc6d

Refactor: Minor updates to MigratableUserDefaultsKey

7d1ae37

Add: documentation to new functions for copying extensions data

7344458

dvdchr approved these changes Nov 28, 2022

View reviewed changes

hassaanelgarem added 2 commits November 28, 2022 10:47

Fix: typo in documentation

6c0b43d

Merge pull request #19666 from wordpress-mobile/fix/19591-extensions-…

ca11d56

…copy-data Jetpack Focus: Copy Extensions' data to JP

hassaanelgarem enabled auto-merge November 28, 2022 08:49

hassaanelgarem removed the [Status] DO NOT MERGE label Nov 28, 2022

hassaanelgarem merged commit 4fadf5c into trunk Nov 28, 2022

hassaanelgarem deleted the fix/19591-extensions-diff-keys branch November 28, 2022 09:17

Uh oh!

Conversation

hassaanelgarem commented Nov 25, 2022

Description

Testing Instructions

Regression Notes

Uh oh!

peril-wordpress-mobile Bot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvdchr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdchr Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

dvdchr Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

momo-ozawa left a comment

Choose a reason for hiding this comment

Uh oh!

staskus Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvdchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peril-wordpress-mobile Bot commented Nov 25, 2022 •

edited

Loading

wpmobilebot commented Nov 25, 2022 •

edited

Loading

wpmobilebot commented Nov 25, 2022 •

edited

Loading

dvdchr left a comment •

edited

Loading

dvdchr Nov 25, 2022 •

edited

Loading

dvdchr Nov 25, 2022 •

edited

Loading

hassaanelgarem commented Nov 27, 2022 •

edited

Loading