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.

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

[MBL-1441] Remove 'Report This Project' Feature Flag #2063

Merged
merged 3 commits into from
May 21, 2024
Merged

[MBL-1441] Remove 'Report This Project' Feature Flag #2063

merged 3 commits into from
May 21, 2024

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented May 20, 2024 •
edited
Loading

📲 What

Removes Report This Project Feature Flag.

🤔 Why

It has been on at 100% for some time now. It's time to say goodbye.

🛠 How

  • Remove flag from RemoteConfigFeature and all subsequent tests and helpers
  • Remove any guards that use this feature flag. There's only one.
  • Verify that the feature is working as expected.

👀 See

Simulator Screen Recording - iPhone 15 Pro Max - 2024-05-20 at 09 11 55

♿️ Accessibility

No changes

🏎 Performance

Same

✅ Acceptance criteria

  • Reporting a project still works as expected from the Project Page.

⏰ TODO

  • Update the flag in remote config to stop sending it to new builds. Eventually we'll be able to delete from there once enough users are updated to the 5.17.0 (next release)

@scottkicks scottkicks self-assigned this May 20, 2024
@scottkicks scottkicks force-pushed the scott/debt/cleanup-report-project-flag branch from b746052 to e5f184b Compare May 20, 2024 18:12
@scottkicks scottkicks marked this pull request as ready for review May 20, 2024 18:39
@scottkicks scottkicks requested review from a team and amy-at-kickstarter and removed request for a team May 20, 2024 18:40
scottkicks
scottkicks commented May 21, 2024
View reviewed changes
Copy link
Contributor Author

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Note: A ton of snapshots needed to be updated. Which seemed surprising because there aren't any noticeable differences visually. At least to me. I also updated the failing tests to use the orthogonalCombos.

@amy-at-kickstarter
Copy link
Contributor

@scottkicks I thiiiink I see the difference in the screenshots - there's an extremely, extremely small change in the spacing in some of the buttons. It's barely visible, though.
insane snapshot diff

Any chance there's an Xcode/iOS SDK version difference that triggered the discrepancy?

@scottkicks
Copy link
Contributor Author

@amy-at-kickstarter, I don't think so. Removing this flag does mean that the Project page's DataSource technically has a new item in it (Report This Project row), so maybe that's adding space here 🤔

amy-at-kickstarter
amy-at-kickstarter approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

LGTM, two non-blocking suggestions:

  1. Update your commit message for the large screenshot update to be more descriptive (just in case someone is investigating these tests in the future, and wondering about the huge update)
  2. Delete any screenshots that were made defunct by swapping to orthogonalCombos

...OS/Features/ProjectPage/Datasource/Datasource/ProjectPageViewControllerDataSourceTests.swift
@@ -719,7 +719,7 @@ final class ProjectPageViewControllerDataSourceTests: XCTestCase {
refTag: nil,
isExpandedStates: nil
)
XCTAssertEqual(3, self.dataSource.numberOfSections(in: self.tableView))
XCTAssertEqual(4, self.dataSource.numberOfSections(in: self.tableView))
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the extra section is the report section?

Kickstarter-iOS/Features/ProjectPage/Controller/ProjectPageViewControllerTests.swift
@@ -106,7 +106,7 @@ internal final class ProjectPageViewControllerTests: TestCase {
fetchProjectRewardsResult: .success([reward])
)

combos(Language.allLanguages, [Device.phone4inch, Device.pad]).forEach { language, device in
orthogonalCombos(Language.allLanguages, [Device.phone4inch, Device.pad]).forEach { language, device in
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed no screenshots were deleted - there should be some that are now unused/defunct, correct?

@amy-at-kickstarter
Copy link
Contributor

@amy-at-kickstarter, I don't think so. Removing this flag does mean that the Project page's DataSource technically has a new item in it (Report This Project row), so maybe that's adding space here 🤔

Hrm, yeah, maybe. Some kind of gremlins in the layout engine? It's definitely unfortunate but seems fairly harmless at least. I didn't realize all these updated pngs were just from the Project page, whew.

@scottkicks scottkicks force-pushed the scott/debt/cleanup-report-project-flag branch from e5f184b to f284d8f Compare May 21, 2024 14:37
@scottkicks
Copy link
Contributor Author

@amy-at-kickstarter

✅ 1. Update your commit message for the large screenshot update to be more descriptive (just in case someone is investigating these tests in the future, and wondering about the huge update)

✅ 2. Delete any screenshots that were made defunct by swapping to orthogonalCombos

@scottkicks scottkicks merged commit fa2bd79 into main May 21, 2024
5 checks passed
@scottkicks scottkicks deleted the scott/debt/cleanup-report-project-flag branch May 21, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amy-at-kickstarter amy-at-kickstarter amy-at-kickstarter approved these changes

Assignees

@scottkicks scottkicks

Labels
tech debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scottkicks @amy-at-kickstarter