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

[MBL-900] Use of AI Tab #1842

Merged
merged 7 commits into from Aug 14, 2023
Merged

[MBL-900] Use of AI Tab #1842

merged 7 commits into from Aug 14, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Aug 9, 2023

📲 What and 🤔 Why

Adding the "Use of AI" tab to support new web, Android tab.

🛠 How

  1. Update the ProjectFragment first with the new aiDisclosure properties. These properties are called on every load of the project page.
  2. Updated the ExtendedProjectProperties with aiDisclosure optional, put in an init so at all places this struct is created we explicitly say "include a value for this property" or no
  3. Made the adapter updates in Project+ProjectFragment which is used to convert raw schema data to our in-app Project model.
  4. ProjectNavigationSelectorViewModel is where we include the tab if the feature is on and available.
  5. Setup the template for the data source to load the aiDisclosure sections and rows in ProjectPageViewControllerDataSource when they are ready.
  6. ProjectNavigationSelectorView and ProjectNavigationSelectorViewModel NavigationSection for the tab itself.

👀 See

After 🦋

Simulator.Screen.Recording.-.iPhone.8.-.2023-08-08.at.20.00.38.mp4

Before 🐛

Simulator.Screen.Recording.-.iPhone.8.-.2023-08-08.at.20.03.32.mp4

✅ Acceptance criteria

  • For projects with tab, it is shown, not otherwise.

@msadoon msadoon added this to the release-5.10.0 milestone Aug 9, 2023
@msadoon msadoon self-assigned this Aug 9, 2023
@msadoon msadoon marked this pull request as ready for review August 9, 2023 01:51
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1842 (ad87197) into main (9a1dc93) will increase coverage by 0.00%.
The diff coverage is 91.21%.

@@           Coverage Diff            @@
##             main    #1842    +/-   ##
========================================
  Coverage   84.52%   84.53%            
========================================
  Files        1271     1272     +1     
  Lines      115144   115287   +143     
  Branches    30657    30693    +36     
========================================
+ Hits        97321    97453   +132     
- Misses      16757    16765     +8     
- Partials     1066     1069     +3     
Files Changed Coverage Δ
...tasource/ProjectPageViewControllerDataSource.swift 85.09% <0.00%> (-0.86%) ⬇️
KsApi/models/lenses/ProjectLenses.swift 16.34% <0.00%> (-0.04%) ⬇️
...jectPage/Views/ProjectNavigationSelectorView.swift 78.53% <66.66%> (-0.29%) ⬇️
...iewModels/ProjectNavigationSelectorViewModel.swift 93.84% <75.00%> (-2.83%) ⬇️
.../adapters/Project+FetchProjectQueryDataTests.swift 95.47% <78.57%> (-0.95%) ⬇️
...raphql/adapters/Project+ProjectFragmentTests.swift 97.30% <89.28%> (-0.54%) ⬇️
...ge/Controller/ProjectPageViewControllerTests.swift 100.00% <100.00%> (ø)
...rce/ProjectPageViewControllerDataSourceTests.swift 96.94% <100.00%> (+0.07%) ⬆️
KsApi/models/ExtendedProjectProperties.swift 100.00% <100.00%> (ø)
...raphql/adapters/Backing+BackingFragmentTests.swift 98.61% <100.00%> (+0.02%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@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.

This looks good. Tested with the feature flag on and off 👍

I just have some general feedback:

  • It would be good to see the GraphAPI changes in a separate PR next time so it’s easier to understand them and verify that any other updates that might come through with our apollo-schema-download.sh script don’t cause unexpected behavior.

  • I think we should still be writing up engineering plans for new features. It will help the team understand the planned implementation and how the app works overall. In this case, we’d have more knowledge/discussion around how these tabs are built making it easier for the team to debug and update in the future independently. Plus, documenting this will make everyone’s life easier down the road.

  • It would be helpful to include in the dev testing steps that the app needs to be run with the AppCenter Beta schema in order to update feature flags in the debug menu.

@@ -15,6 +16,7 @@ public enum NavigationSection: Int, CaseIterable {
case .environmentalCommitments: return Strings.Environmental_commitments()
case .faq: return Strings.Faq()
case .overview: return Strings.Overview()
case .aiDisclosure: return "Use of AI" // FIXME: Should be `Strings.Use_Of_AI` in console once translations are done.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a ticket for this already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not, this is kind of a "rushed" feature. So we didn't get much lead time here to implement and release by end of August. So doing some imperfect planning here for sure.

@msadoon
Copy link
Contributor Author

msadoon commented Aug 14, 2023

This looks good. Tested with the feature flag on and off 👍

I just have some general feedback:

* It would be good to see the GraphAPI changes in a separate PR next time so it’s easier to understand them and verify that any other updates that might come through with our `apollo-schema-download.sh` script don’t cause unexpected behavior.

* I think we should still be writing up engineering plans for new features. It will help the team understand the planned implementation and how the app works overall. In this case, we’d have more knowledge/discussion around how these tabs are built making it easier for the team to debug and update in the future independently. Plus, documenting this will make everyone’s life easier down the road.

* It would be helpful to include in the dev testing steps that the app needs to be run with the AppCenter Beta schema in order to update feature flags in the debug menu.
  • Well this GraphQL change is related to the feature, but I take your point. In an ideal world, yes all GraphQL changes would be separate. In this comment, I discuss why non breaking changes can be included in any PR. To elaborate, we can't really stop/version our graphql schema yet. If it changes, every build we do is going to include that schema change. So if we don't include the schema change the latest versions of our app will not sync up with the latest versions of the public GraphQL API. Example: let's say tomorrow they change triggerThirdPartyEvents mutation to have some new mandatory properties. Assuming that API is now public, if we ship our app without that schema change, the mutation will crash the app, because it's breaking the build and we ignored it. Unless we create a new PR just to address this change, which is fine, but again time consuming and this is a rushed feature.

  • Engineering plans are more for new features. Having built this feature out over a year ago I'm comfortable making similar additions to this page. I did start with an engineering plan initially - several (1 2). Given it's another "tab" similar to the other ones, an engineering plan is probably not required. Also this is kind of "rushed" feature. We only got about two weeks to build out the entire tab along with all the other sprint work.

  • I'm also pushing eng plans in general to reduce repetitive mistakes. They are necessary to reduce instances of missing previously discussed ideas, lack of QA, detail, testing and code quality. With a shared mindset of how to build new features and update existing ones, we grow confident in each other's code and reviews take much less time and energy. To drive towards that ideal, more time in the discussion/planning phase is necessary.

  • Yea I purposefully didn't include "dev testing" because it didn't seem necessary as this is a somewhat repetitive task (ie. adding a feature flag)

TLDR: Please bear with me, this is a rushed feature.

@msadoon msadoon merged commit baffd7c into main Aug 14, 2023
7 checks passed
@msadoon msadoon deleted the mbl-900/new-tab-use-of-ai branch August 14, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants