Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[NT-930] Add Project Creator Details Query and VM outputs #1086

Conversation

justinswart
Copy link
Contributor

📲 What

Adds a query for us to fetch some additional project creator details from GraphQL when a project page loads.

Note: This emits what we expect to need to configure the cell that will display this data, that is the ProjectCreatorDetailsEnvelope result and an isLoading boolean to indicate whether we are still fetching the results.

🤔 Why

As part of an experiment that we are running we would like to display some additional data to surface trust indicators to users. These values are not available from the v1 API so we are fetching them from GraphQL.

🛠 How

  • Added ProjectCreatorDetailsEnvelope.
  • Added fetchProjectCreatorDetails to ServiceType.
  • Added some types to ProjectPamphletContentViewModel to improve the frequently changing interface of its datasource loading output.
  • Updated GraphSchema.
  • Added tests.

✅ Acceptance criteria

  • Upon a project page loading observe that (nil, true) is emitted to put the cell in an initial loading state.
  • Once the GraphQL request successfully completes observe that (result, false) is emitted.
  • If the request fails observe that (nil, false) is emitted.

@@ -39,7 +39,7 @@ internal final class ProjectPamphletContentDataSource: ValueCellDataSource {
}

self.set(
values: [projectAndRefTag],
values: [(project, refTag)],
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 will update ProjectPamphletMainCell to be configured with all three values (or just ProjectPamphletContentData).

fileprivate let fetchProjectsResponse: [Project]?
fileprivate let fetchProjectsError: ErrorEnvelope?

fileprivate let fetchProjectCreatorDetailsResult: Result<ProjectCreatorDetailsEnvelope, GraphError>?

fileprivate let fetchProjectNotificationsResponse: [ProjectNotification]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alphabetized this so it moved around in a few places. But I now realize a bunch of things are still out of place so I might revert this change and rather fix the ordering separately.

.ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
.map { result in (result, false) }
.prefix(value: (nil, true))
.demoteErrors(replaceErrorWith: (nil, false))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this errors out we just return nil and tell the cell to stop loading. We will do additional work to not even activate the experiment if we don't have this data.

@@ -306,6 +302,14 @@ public struct Service: ServiceType {
return requestPagination(paginationUrl)
}

public func fetchProjectCreatorDetails(query: NonEmptySet<Query>) -> SignalProducer<ProjectCreatorDetailsEnvelope, GraphError> {
Copy link
Collaborator

@nativeksr nativeksr Feb 27, 2020

Choose a reason for hiding this comment

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

🚫 Line should be 110 characters or less: currently 130 characters
line_length Service.swift:305

@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

self.loadProjectIntoDataSourceProject
.assertValues([project], "Load the full project into the data source.")
self.loadProjectIntoDataSourceRefTag.assertValues([.discovery], "Load the refTag into the data source.")
self.loadMinimalProjectIntoDataSource.assertValues([], "Do not load the minimal version of the project.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Line should be 110 characters or less: currently 111 characters
line_length ProjectPamphletContentViewModelTests.swift:501

Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a couple questions. Also it seems that there are some Line Length violations.

@@ -192,17 +193,18 @@ public enum Query {
}
}

public enum User {
public indirect enum User {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we are enabling indirection for all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is because of the recursive relationship between Project <-> User. Without this it gives this error:
image

@@ -6,30 +6,39 @@ import ReactiveSwift
import XCTest

final class ProjectPamphletContentViewModelTests: TestCase {
fileprivate let vm: ProjectPamphletContentViewModelType = ProjectPamphletContentViewModel()
fileprivate var vm: ProjectPamphletContentViewModelType!
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change necessary? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Sometimes it is for the scheduler to work correctly but in this case it wasn't. Thanks 👍

Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

GIF

@justinswart justinswart merged commit b511e61 into NT-863-feature-creator-details-experiment Feb 27, 2020
@justinswart justinswart deleted the NT-930-add-project-creator-details-query branch February 27, 2020 23:47
ifbarrera pushed a commit that referenced this pull request Mar 18, 2020
* [NT-878, NT-873] Add Experiment and Read More is a full-width button (#1073)

* [NT-876] Add Pledge CTA to ProjectDescriptionViewController (#1072)

* [NT-876] Pop ProjectPamphlet to root after project is backed (#1075)

* [NT-876] Add Project Campaign Experiment Tests (#1076)

* Hide the back this project button to creators of their own projects (#1078)

* [NT-877] Project page Optimizely Events (#1079)

* Add loader to read more button in second variant (#1077)

* [NT-930] Add Project Creator Details Query and VM outputs (#1086)

* [NT-940] Activate Creator Details Experiment (#1088)

* [NT-931] Add creator byline view (#1089)

Co-authored-by: Justin Swart <justinswart@users.noreply.github.com>

* [NT- 956] CreatorByline View Model and Tests (#1095)

* [NT-937] Creator Details Shimmer Loading View (#1097)

* [NT- 939] Creator details tracking events (#1098)

* translated copy (#1111)

Co-authored-by: Christella <cdolm92@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants