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

Expand AdHoc build workflow, add debug bookmarks screen #3086

Merged
merged 13 commits into from
Jul 17, 2024

Conversation

bwaresiak
Copy link
Collaborator

@bwaresiak bwaresiak commented Jul 16, 2024

Task/Issue URL: https://app.asana.com/0/414235014887631/1207821981275160/f

https://app.asana.com/0/856498667320406/1207823615637251/f

Description:

Expand AdHoc build workflow to have an option of alpha and regular builds. Add debug bookmarks screen.

Steps to test this PR:

Check the workflow file for any obvious mistakes, validate history of executions for any mistakes.

Validate debug screen works as expected.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@samsymons samsymons self-requested a review July 16, 2024 14:28
@bwaresiak bwaresiak changed the title Add debug bookmarks screen Expand AdHoc build workflow, add debug bookmarks screen Jul 16, 2024
Comment on lines 13 to 20
is-alpha:
description: "Is alpha build"
type: choice
required: true
default: 'true'
options:
- true
- false
Copy link
Contributor

@ayoy ayoy Jul 16, 2024

Choose a reason for hiding this comment

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

GHA supports boolean type so we can use it here. It would then use a checkbox in the UI, instead of a dropdown. The call site doesn't need updating:

Suggested change
is-alpha:
description: "Is alpha build"
type: choice
required: true
default: 'true'
options:
- true
- false
is-alpha:
description: "Whether the build uses Alpha or Release configuration"
type: boolean
required: true
default: true

Copy link
Collaborator Author

@bwaresiak bwaresiak Jul 16, 2024

Choose a reason for hiding this comment

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

I know, but I've seen people complaining that sometimes these behave like booleans, sometimes like strings, there's a whole thread actions/runner#1483 so I'd prefer to explicitly use strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's why the call site still needs to treat them as strings :) just pointing it out because it's possible to use a checkbox here, although on a second thought we could make it a dropbox with more descriptive options, like "Configuration: Alpha/Release". I'm not strong on any of my suggestions though, as long as the message is clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tweaked the options and description a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that looks much clearer. Sorry to be a pain, but I'd also rename is-alpha now that it's not a boolean anymore. How about build-type? configuration is the most correct, but it also is a broad term in general and may be confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, done :)

@bwaresiak bwaresiak requested a review from ayoy July 16, 2024 17:39
Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

@bwaresiak When testing the debug menu I saved a couple favorites, but the debug menu doesn't show their metadata - it does show the root folders correctly.

Bookmarks

Then when deleting all bookmarks and re-running the app, I ran into this assertion failure:

Assertion Failure

A second run of the app worked, and I got the root folders recreated, but I still wasn't getting bookmark metadata in the debug menu.

@bwaresiak
Copy link
Collaborator Author

@samsymons thanks, I've fixed the context (as it was being deallocated, making entries lose the content) and removed the delete option - since bookmarks can be managed by sync we don't want to just wipe them from db.

@bwaresiak bwaresiak requested a review from samsymons July 17, 2024 07:54
Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

LGTM :)

@bwaresiak
Copy link
Collaborator Author

Thank you!

@bwaresiak bwaresiak merged commit 436449c into main Jul 17, 2024
14 checks passed
@bwaresiak bwaresiak deleted the bartek/debug-bookmarks-screen branch July 17, 2024 19:06
samsymons added a commit that referenced this pull request Jul 19, 2024
# By Christopher Brind (9) and others
# Via Chris Brind (1) and GitHub (1)
* main: (48 commits)
  Reduce VPN manager instances (#3097)
  Update BSK for Mac RMF changes (#3107)
  [DuckPlayer] 7- Open Settings (#3110)
  [DuckPlayer] 6 - Init updates and Watch on YouTube (#3066)
  New Tab Page Shortcuts section (#3104)
  Fix VPN configuration removal to stop the tunnel (#3099)
  avoid resizing webview when keyboard shows/hides (#3094)
  Add support for skipping sending usage pixels for remote messages (#3106)
  Bump submodules/privacy-reference-tests from `a242bf0` to `afb4f61` (#3096)
  Updates BSK to 171.2.3
  Remove print (#3101)
  Update Package.resolved file (#3102)
  New Tab Page favorites section (#3083)
  Properly compare actual value of the entitlement check (#3100)
  fix ui tests broken by new onboarding and use shared setup flow (#3081)
  AdHoc lane: Make proper assignment to variable (#3095)
  Expand AdHoc build workflow, add debug bookmarks screen (#3086)
  Update BSK to latest - for macOS fix to AdAttribution (#3084)
  Revert "Fix tests"
  Fix tests
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/NetworkProtectionStatusViewModel.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants