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-971] Submit Report Project View #1859

Merged
merged 28 commits into from
Oct 2, 2023
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Sep 28, 2023

📲 What

New SwiftUI View that displays a form allowing users to report a project.

🤔 Why

Our iOS app currently violates the User Generated Content Policy in the App Store. Specifically, we need (at the project screen level):

  • A method for filtering objectionable content
  • A mechanism for users to flag objectionable content
  • A mechanism for users to block abusive users

This form should appease Apple

🛠 How

  • Builds a new SwiftUI view based using android's implementation as an example.
  • Uses our MVVM pattern + Combine to handle state changes and submit the form using our createFlagging graphql mutation
    • We don't have official SwiftUI/Combine standards yet so I mostly followed the existing pattern used in our ChangeEmailView (Also a SwiftUI view)
  • The form displays the user's email and projectID for the user's benefit only, and a text box for adding details as to why they are reporting.
  • Displays a Message banner to notify the user when the submission was successful or not
  • On successful submission, we auto dismiss these report project views and reload the project page so that it shows the "already reported" label and keeps users from reporting a project more than once.

👀 See

Simulator Screen Recording - iPhone 14 - 2023-09-28 at 12 24 09

✅ Acceptance criteria

Happy Path

  • If the Report Project feature flag is on and the user is logged in, they will see the Report this project option.
  • The form displays the logged-in user's email and project URL. This is for the user's benefit and is not editable.
  • Tapping save, submits a report, shows the success message, and then navigates back to the Project page where it will display the new "already reported this project" label
  • Users cannot submit a report more than once for the same project at any point

Sad Path

  • If there is an issue when tapping save, the error message displays but users are given the opportunity to submit again.

* UITableView cell will be added to the Project Page Table View
* Used to display a new swiftui view for the report this project label and already reported label
ReportProjectLabelView (SwiftUI View) re-renders when its text labels contain hyperlinks. Since we're rendering hyperlinks with the helper in Text+HTML.swift, this seems to cause a `precondition failure: cyclic graph` crash. This check prevents that by making sure the cell is only configured once on dequeue.
we don't need to hide it now that there is a new section below it.
* UITableView cell will be added to the Project Page Table View
* Used to display a new swiftui view for the report this project label and already reported label
no viewmodel or service functionality yet.
We might be changing this approach with Combine as we solidify standards later. For now, I'm following existing patterns
* this won't work just yet. still need to pass the projectID to this view

* submits a report using createFlagging mutation
* shows a message banner on successful/unsuccessful response
* updates save button/loading button as needed
clientMutationId is optional so we can just pass nil here
* this is to keep users from reporting a project more than once
need to re-record snapshots since the label is now behind the feature flag
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #1859 (fb35e11) into main (8f40c4c) will decrease coverage by 0.13%.
The diff coverage is 28.31%.

@@            Coverage Diff             @@
##             main    #1859      +/-   ##
==========================================
- Coverage   83.95%   83.83%   -0.13%     
==========================================
  Files        1289     1292       +3     
  Lines      117118   117304     +186     
  Branches    31158    31223      +65     
==========================================
+ Hits        98327    98338      +11     
- Misses      17702    17874     +172     
- Partials     1089     1092       +3     
Files Coverage Δ
...rce/ProjectPageViewControllerDataSourceTests.swift 97.42% <100.00%> (-0.02%) ⬇️
KsApi/models/ReportProjectInfoListItem.swift 0.00% <ø> (ø)
KsApi/mutations/inputs/CreateFlaggingInput.swift 100.00% <ø> (ø)
Library/ViewModels/ProjectPageViewModelTests.swift 98.16% <100.00%> (+<0.01%) ⬆️
Library/ViewModels/ProjectPageViewModel.swift 97.04% <0.00%> (ø)
...ibrary/ViewModels/MessageBannerViewViewModel.swift 0.00% <0.00%> (ø)
...y/ViewModels/ReportProjectFormViewModelTests.swift 89.47% <89.47%> (ø)
...es/ProjectPage/Views/Cells/ReportProjectCell.swift 0.00% <0.00%> (-97.15%) ⬇️
...tasource/ProjectPageViewControllerDataSource.swift 82.17% <0.00%> (-1.63%) ⬇️
...ectPage/Controller/ProjectPageViewController.swift 49.77% <0.00%> (-0.54%) ⬇️
... and 3 more

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

@scottkicks scottkicks marked this pull request as ready for review September 28, 2023 20:07
Copy link
Contributor

@ifosli ifosli left a comment

Choose a reason for hiding this comment

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

I'd like to discuss this more after the cut; I don't think we have time to go into what we want our standards to be right now (I think this should be using Combine instead of reactive swift, for example).

I wasn't able to test this fully (I still don't have an admin password so I can't check if report got sent) but the in-app part looks good. I do think that the "Save" button needs to be called "Send" or "Submit" instead, but I guess that will also need to be a fast follow instead.

So with the caveat that I don't think we have time to explore the best way to do SwiftUI or combine right now, this is good, but I'd love to revisit the decisions made here asap. (I'm not saying the SwiftUI part of it is bad; I am saying it's not following conventions that I'm used to, and I'd need time to dig into why it's structured the way it is to figure out if I agree with it or not. Either way, it's good enough for now.)

@scottkicks scottkicks merged commit 9bda2ba into main Oct 2, 2023
7 checks passed
@scottkicks scottkicks deleted the mbl-971-submit-report-view branch October 2, 2023 13:26
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.

2 participants