-
Notifications
You must be signed in to change notification settings - Fork 278
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
Ensure that the Ad Blocking Recovery
is used consistently across the plugin code
#7164
Comments
Ad Blocking Recovery
is used consistently across the plugin code
Hey, I would like to resolve this issue. |
Hi @promise-dash, thanks for your interest here. This issue is scheduled to go through our usual development process, but if you create a PR we'll happily consider it. However please do bear in mind this issue should most likely be implemented after the other Ad Blocker Detection issues to ensure that all relevant code is updated. |
Hey @kuasha420, this IB is LGTM. However I notice that this issue doesn't have any dependencies, I think we should add some to ensure that it doesn't collide with or miss any new additions that use the outdated naming scheme. We could either check through the remaining ABD issues for those which reference the old names and add those as dependencies, or, what I think would be safer would be to schedule this after all of the ABD issues are complete. Or a third option could be to proactively go and update any existing issues to clear up the names in their definitions. What do you think? |
@techanvil @kuasha420 Totally up to you from an engineering perspective, but if we do decide we'd prefer to wait until ABD Is complete, I'd say that this would be something that we do post-launch – e.g. we finish engineering in Sprint 104, go through the whole Bug Bash/release process over the next several sprints, and then come back to this later in August after the initial release. Otherwise, pre-launch testing will be tricky because we'd still be making changes across the code for this feature. If we don't want to wait that long, we'd want to get this done in Sprint 104 along with all of the other work on this feature. Let me know what you think! |
@bethanylang you raise a good point, however I would advocate for getting this done pre-bug bash, as otherwise a change like this which touches a lot of code would ideally require a regression test across the ABD feature anyway so it would be more efficient to get it in prior. The thing is, as this is just essentially a load of search and replacing with no logical changes it should be pretty quick to implement, as such I'd suggest we should get it done immediately preceding the bug bash. Interested to hear @kuasha420's thoughts on this too. |
@techanvil Yep, agreed that Sprint 104 would be the most efficient, but will leave it up to @kuasha420 to confirm! |
Hi, we should definitely do this before launch, because it will change a few settings name. If we do it post launch, we'll need a Settings migration which is absolutely not desirable for such trivial change. As for dependencies, we can get by without it I think, as getting this done and merged will probably create merge conflict or test failures in the in-flight PR which will get naturally resolved and the rest of the tickets will just use the new names and variables naturally (although some part of their IB will become less accurate). That way nothing will be held up. Or we can make it dependant on 6962 and 6966, that way there will be less friction on other tickets but this one will likely not start until the very end of 104 or will likely get pushed over to 105. However as TOM said, the change here is not very high effort and can probably be done very quickly upon start. TLDR: We should do it before launch and preferably before bug bash, but the when is totally up to you. Cheers |
Thanks @kuasha420, this is a really helpful explanation! I'm going to schedule this for Sprint 104 and add an @kuasha420 Do you mind assigning this one to yourself once it gets to EB, since you're familiar with it? Then you can plan to work on it w/c 26 June. TY both! |
@techanvil Now this is sorted, feel free to have a look over and assign this one to me in EB. Cheers! |
Thanks @kuasha420 & @bethanylang, SGTM! In that case, IB ✅ and I'll assign this to @kuasha420 in EB. 🎉 |
QA Update: ✅Verified:
|
Feature Description
For the most part, the feature behind the
adBlockerDetection
feature flag is referred as Ad Blocking Recovery in the UI and code- namely, the copy, components, API routes, and other variables and pieces of codes. However, in a few place,Ad Blocker Detection
is used, namely the Ad Blocking Recovery related AdSense module settings.Besides the instances with the feature flag ie.
isAdBlockerDetectionEnabled
, all other usage of theAd Blocker Detection
(or its variantsadBlockerDetection
andAdBlockerDetection
) andAd Blocker Recovery
(or its variantsadBlockerRecovery
andAdBlockerRecovery
) should be refactored to use the actual feature name.Acceptance criteria
AdBlockerRecovery
or it's other case variations should be replaced withAdBlockingRecovery
across the code base, including code, comments, copies and file names.AdBlockerDetection
or it's other case variations should be replaced withAdBlockingRecovery
across the code base, including code, comments, copies and file names.adBlockerDetection
feature flag and related variables. ie.isAdBlockerDetectionEnabled
.Implementation Brief
AdBlockerRecovery
and all it's case variations, and replace them with the correct string as described in the IB, keeping the case intact.AdBlockerRecovery
ad-blocker-recovery
adBlockerRecovery
ad_blocker_recovery
Ad Blocker Recovery
AdBlockerDetection
and all it's case variations, and replace them with the correct string as described in the IB, keeping the case intact.AdBlockerDetection
ad-blocker-detection
adBlockerDetection
ad_blocker_detection
Ad Blocker Detection
Test Coverage
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: