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

Feature/in app review system #148

Merged

Conversation

IvanStepanok
Copy link
Contributor

@IvanStepanok IvanStepanok commented Nov 3, 2023

RPReplay_Final1699274174.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-03.at.13.24.57.mp4

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 3, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @IvanStepanok! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[header] the system bar disappears if a user used the rate us pop-up
[rate_us] the user isn't able to submit a feedback in the landscape mode
[email_feedback] the email template formatting must have a user/device info
[email_agent] iOS agent displays bit a user cannot use it (the icon isn't clickable)
@sdaitzman
Copy link

This looks great to me! It would be ideal to collect feedback directly in-app, but that requires some backend communication and is probably not in-scope for now.

Two questions about the interface:

  • What currently triggers this dialog? Is it timing-based? Could it be triggered only when a video is not actively playing, maybe immediately after a video?
  • Is the mail client picker that appears at the bottom custom? I believe iOS supports changing default mail apps now, so I think it would be better to use the user's chosen mail client without prompting them to choose one

@mumer92
Copy link
Contributor

mumer92 commented Nov 13, 2023

@rnr one feedback i have to to make a separate directory and place 3rd party libraries we are using in project to that directory. in that way, we will be able to see what external dependencies we are using, it will have better organization of of code base. what are your thoughts?

@rnr
Copy link
Contributor

rnr commented Nov 13, 2023

@rnr one feedback i have to to make a separate directory and place 3rd party libraries we are using in project to that directory. in that way, we will be able to see what external dependencies we are using, it will have better organization of of code base. what are your thoughts?

@mumer92 do you mean something else except Pods and Swift Packages (I believe these two Sources are organised well already)? Thank you

@mumer92
Copy link
Contributor

mumer92 commented Nov 13, 2023

@rnr one feedback i have to to make a separate directory and place 3rd party libraries we are using in project to that directory. in that way, we will be able to see what external dependencies we are using, it will have better organization of of code base. what are your thoughts?

@mumer92 do you mean something else except Pods and Swift Packages (I believe these two Sources are organised well already)? Thank you

My understanding is ThirdPartyMailClient external library? If it is, we should create a separate folder on root level and place files to that folder, e.g. ExternalLibraries/ThirdPartyMailClient and same with other third party files that are added to project.

@volodymyr-chekyrta
Copy link
Contributor

This looks great to me! It would be ideal to collect feedback directly in-app, but that requires some backend communication and is probably not in-scope for now.

Two questions about the interface:

  • What currently triggers this dialog? Is it timing-based? Could it be triggered only when a video is not actively playing, maybe immediately after a video?
  • Is the mail client picker that appears at the bottom custom? I believe iOS supports changing default mail apps now, so I think it would be better to use the user's chosen mail client without prompting them to choose one

Yes, it's based on timing here, it appears at 99.9% of the video.

After chatting with @marcotuts, we thought it's a good idea to give users a choice.
Most people have the Apple client by default, but there's a bunch who prefer Gmail.

We can make changes if needed.

@volodymyr-chekyrta
Copy link
Contributor

@rnr one feedback i have to to make a separate directory and place 3rd party libraries we are using in project to that directory. in that way, we will be able to see what external dependencies we are using, it will have better organization of of code base. what are your thoughts?

@mumer92 do you mean something else except Pods and Swift Packages (I believe these two Sources are organised well already)? Thank you

My understanding is ThirdPartyMailClient external library? If it is, we should create a separate folder on root level and place files to that folder, e.g. ExternalLibraries/ThirdPartyMailClient and same with other third party files that are added to project.

That's not the library we're going to update in the future.
Here it's just a part of the App Review feature, so it's located there.

}

func requestReview() {
SKStoreReviewController.requestReview()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @IvanStepanok
As I know SKStoreReviewController can be showed three times per year only
https://developer.apple.com/documentation/storekit/requesting_app_store_reviews
All other times 'Rate Us' button won't work, right?
Thank you

@mumer92
Copy link
Contributor

mumer92 commented Nov 14, 2023

That's not the library we're going to update in the future.
Here it's just a part of the App Review feature, so it's located there.

@volodymyr-chekyrta ThirdPartMailer is available as a library via pod, i believe this is the link to github project. https://github.com/vtourraine/ThirdPartyMailer

My main feedback is to separate out the external or third party code we are using in our code base, e.g in a separate folder called External Libraries and place all the code, not written by and we think is not going to be changed by us there, this way we can have clarity over the code of what is written by us, what code needs to be reviewed, and what external library or code files we are using.

There are other instances of third party code that we are relying in our code base, e.g.

@marcotuts
Copy link

A new notes - can we trigger the rating once the video ends instead of at 99.99% - this way the interrupt isn't near the end of an activity but right as it ends.

Similarly on the note about rating visibility, I can review the code next but ensuring this doesnt appear more than a certain number of times is important. the requirements only specified that the modal should appear at most ever 3 months, but if you chose to rate the app we should actually have a timer that is less frequent / perhaps yearly? Thoughts on this change in logic / addition to the logic @IvanStepanok ?

I wasn't familiar with the 3 limit max @rnr thanks for f lagging that.

@volodymyr-chekyrta
Copy link
Contributor

@volodymyr-chekyrta ThirdPartMailer is available as a library via pod, i believe this is the link to github project. https://github.com/vtourraine/ThirdPartyMailer

@mumer92,
We know the library is available via pods, yet it is not currently under active development.
Its most recent update was on April 18, 2022, and it does not utilize the latest version of Swift.
Therefore, we have opted to implement only the necessary features instead of relying on this framework.

@volodymyr-chekyrta
Copy link
Contributor

A new notes - can we trigger the rating once the video ends instead of at 99.99% - this way the interrupt isn't near the end of an activity but right as it ends.

99.99% of the time, this serves as a workaround for the YouTube player, which does not consistently report full 100% progress.
However, we've conducted tests on a wide array of videos and found that typically, the final second marked by 99.99% progress does not contain any significant content.

@volodymyr-chekyrta
Copy link
Contributor

@marcotuts, we didn't see major releases coming more than three times a year, but that's a good point you've raised with @rnr.
We'll add an additional check to the logic.
What do you think works better - limiting it to once every four months or every six months?

@sdaitzman
Copy link

sdaitzman commented Nov 14, 2023

Yes, it's based on timing here, it appears at 99.9% of the video.

After chatting with @marcotuts, we thought it's a good idea to give users a choice. Most people have the Apple client by default, but there's a bunch who prefer Gmail.

We can make changes if needed.

That makes sense, I think it's reasonable to include the choice. In that case, the UI looks good to me!

@saeedbashir
Copy link
Contributor

We know the library is available via pods, yet it is not currently under active development.

@volodymyr-chekyrta I guess what Umer is saying is that there should be clarity in what code we wrote and what code we are using from a third-party or external source.

And for that clarity, he was suggesting to create a folder named (ExternalLibraries/3rdPartyLibraries) at root in the framework where we are using a third-party library and place the 3rd party code in that folder instead of having it in the subdirectory. For example, in the rating feature, a folder named ExternalLibraries would be created in the Core framework and ThirdPartyMailClient.swift placed in that folder.

@volodymyr-chekyrta
Copy link
Contributor

We know the library is available via pods, yet it is not currently under active development.

@volodymyr-chekyrta I guess what Umer is saying is that there should be clarity in what code we wrote and what code we are using from a third-party or external source.

And for that clarity, he was suggesting to create a folder named (ExternalLibraries/3rdPartyLibraries) at root in the framework where we are using a third-party library and place the 3rd party code in that folder instead of having it in the subdirectory. For example, in the rating feature, a folder named ExternalLibraries would be created in the Core framework and ThirdPartyMailClient.swift placed in that folder.

I understand, but I'm not quite sure why we need to create a new folder and relocate these files into it for this specific feature.
I strongly believe that this is redundant in this particular situation.

@saeedbashir
Copy link
Contributor

but I'm not quite sure why we need to create a new folder and relocate these files into it for this specific feature

There are multiple reasons why we are suggesting moving 3rd party libraries to a central location:

  • Most of the open-source libraries are available under the MIT license; in given case, ThirdPartyMailer is available under MIT license which means we should attribute the author
  • We don't need to review the third-party code, but if there isn't any indication of 3rd-party code then we will end up reviewing that code as well
  • It will be much easier to list down all the third-party libraries that we are using.

@volodymyr-chekyrta
Copy link
Contributor

  • Most of the open-source libraries are available under the MIT license; in given case, ThirdPartyMailer is available under MIT license which means we should attribute the author

@saeedbashir Could you please specify how we should attribute the author?
An example from the old repo would be great.

@saeedbashir
Copy link
Contributor

  • Most of the open-source libraries are available under the MIT license; in given case, ThirdPartyMailer is available under MIT license which means we should attribute the author

@saeedbashir Could you please specify how we should attribute the author? An example from the old repo would be great.

There are two steps involved, but the first is a must:

  1. Add the library or files with the author details along with the license.
  2. Highlight the usage of the library somewhere in the app or codebase.

We were using the first step. You can find details about all the external libraries that were being used in edX as local libraries here

Added a check to ensure that the rating window is displayed no more than once every 4 months.

Corrected copyright in ThirdPartyMailer
@volodymyr-chekyrta
Copy link
Contributor

@marcotuts, upgraded logic in the last commit, so now the review popup only shows once every four months.

}

func requestReview() {
SKStoreReviewController.requestReview()
Copy link
Contributor

Choose a reason for hiding this comment

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

app has minimum target of iOS 15, this api was deprecated in iOS 14, how about using the updated api?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace to @Environment(\.requestReview) var requestReview and move logic from AppReviewViewModel to AppReviewView.

func openMailClient(_ with: ThirdPartyMailClient) {

let osVersion = UIDevice.current.systemVersion
let appVersion = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String ?? ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Bundle.main.infoDictionary?["CFBundleShortVersionString"] is being used at multiple places, i think we should centralize this somewhere, what are your thoughts?

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'll need to think about this as part of the next refactoring.

})
} else {
VStack(spacing: 20) {
if viewModel.state == .thanksForFeedback || viewModel.state == .thanksForVote {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using switch here instead of if else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done!

move requestReview logic to AppReviewView
replace if/else logic to switch in AppReviewViewModel
Conflicts:
	Core/Core/Data/CoreStorage.swift
push SKStoreReviewController logic to extension for iOS 15 compatibility
@volodymyr-chekyrta volodymyr-chekyrta merged commit 0d28f7d into openedx:develop Nov 17, 2023
2 checks passed
@openedx-webhooks
Copy link

@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@volodymyr-chekyrta volodymyr-chekyrta deleted the feature/in-app-review-system branch November 17, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants