Limit reviews upto date specified. #13040

scorpion35 · 2018-08-06T13:32:17Z

Solution: Added an optional upto_date parameter to get_review methods. By limiting get_reviews upto a certain date, the time to return from the method is reduced from hours to seconds!

Examples -
Before: Time taken to get all reviews: ~ upto 2 hours (best case). Up to 5 hours (worst case), and the script crashes in the end.

2018-08-03 11:14:33 PM -04 EDT - INFO - [apple_reviews] | Finding all available reviews for "bundleid1" ...
2018-08-04 02:18:39 AM -04 EDT - INFO - [apple_reviews] | Found 90,000 reviews.
...
2018-08-03 11:14:33 PM -04 EDT - INFO - [apple_reviews] | Finding all available reviews for "bundleid2" ...
2018-08-04 02:18:39 AM -04 EDT - ERROR - [apple_reviews] | It seems the script crashed in iteration 1 while getting reviews for App: "bundleid2".

After: Time taken to get reviews up to a specified date: ~ 2 secs (best case) to ~ 30 secs.

2018-08-05 12:41:00 PM -04 EDT - INFO - [apple_reviews] | Finding all available reviews ...
2018-08-05 12:41:02 PM -04 EDT - INFO - [apple_reviews] | 	Found 100 reviews upto date "2018-07-01"
...
2018-08-05 12:40:12 PM -04 EDT - INFO - [apple_reviews] | Finding all available reviews ...
2018-08-05 12:40:38 PM -04 EDT - INFO - [apple_reviews] | 	Found 1700 reviews upto date "2018-07-01"

googlebot · 2018-08-06T13:32:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

joshdholtz

Thanks for making this PR 😊 I probably left too many comments but date stuff is weird 😊

BTW, we probably also want to filter all_reviews with the up to the date specified in the parameter. Right now the date logic is only breaking the loop but not filtering the any that may actually be before the date 🙃

joshdholtz · 2018-08-07T12:03:07Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -371,6 +371,12 @@ def get_reviews(app_id, platform, storefront, version_id)

        r = request(:get, rating_url)
        all_reviews.concat(parse_response(r, 'data')['reviews'])
+
+        last_review_date = Date.parse(Time.at(all_reviews[-1]['value']['lastModified']/1000).to_s).to_date
+        if last_review_date < DateTime.parse(upto_date).to_date


Can we move the DateTime.parse(upto_date).to_date to a variable before the loop do so that this operation oly needs to happen once 😊

Also, we should be able to just use Time.parse and then remove the Date.parse on line 375 (see comment on 375)

Done! e846dd1#diff-64843dee7bf6cf0342bd0e57d9b43c2b

joshdholtz · 2018-08-07T12:04:20Z

spaceship/lib/spaceship/tunes/app_ratings.rb

@@ -46,8 +46,8 @@ def average_rating
      end

      # @return (Array) of Review Objects
-      def reviews(store_front = '', version_id = '')
-        raw_reviews = client.get_reviews(application.apple_id, application.platform, store_front, version_id)
+      def reviews(store_front = '', version_id = '', upto_date = '')


Can we make upto_date = '' default to nil instead?

Good idea. Done! e846dd1#diff-ba47141cdb85eda8d52ad6ea553966fb

joshdholtz · 2018-08-07T12:18:33Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -371,6 +371,12 @@ def get_reviews(app_id, platform, storefront, version_id)

        r = request(:get, rating_url)
        all_reviews.concat(parse_response(r, 'data')['reviews'])
+
+        last_review_date = Date.parse(Time.at(all_reviews[-1]['value']['lastModified']/1000).to_s).to_date


We should be able to just do Time.at(all_reviews[-1]['value']['lastModified']/1000) and compare on time (see comment below)

Done! e846dd1#diff-64843dee7bf6cf0342bd0e57d9b43c2b

Please take a look and let me know if you'd like to change it to something else.

joshdholtz · 2018-08-07T12:19:18Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -358,7 +358,7 @@ def get_ratings(app_id, platform, version_id = '', storefront = '')
      parse_response(r, 'data')
    end

-    def get_reviews(app_id, platform, storefront, version_id)
+    def get_reviews(app_id, platform, storefront, version_id, upto_date)


Can you change this upto_date to default to nil like upto_date = nil to make this backwards compatible? There will need to be an if check below in the logic to check to make sure this upto_date isn't nil then but that logic is okay to have in here 😊

Done! e846dd1#diff-ba47141cdb85eda8d52ad6ea553966fb

scorpion35 · 2018-08-07T23:49:34Z

@joshdholtz Have made changes suggested by you! Kindly take a look and let me know if you need me to do anything else.

joshdholtz

Got a few more small comments! It also looks like the tests are failing and so is the linting. So if you could also ...

Fix existing tests
Add new tests that use the new parameter
Run rubocop (the linter) by running rubocop -a && rubocop -D
Sign CLA

Looking forward to getting this merged after these steps 😊

joshdholtz · 2018-08-08T00:44:21Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -371,13 +374,20 @@ def get_reviews(app_id, platform, storefront, version_id)

        r = request(:get, rating_url)
        all_reviews.concat(parse_response(r, 'data')['reviews'])
+        last_review_date = Time.at(all_reviews[-1]['value']['lastModified']/1000)
+
+        break if last_review_date < upto_date && !upto_date.nil?


I would probably make this break and filter here something like this...

if upto_date && last_review_date < upto_date all_reviews = all_reviews.select { |review| Time.at(review['value']['lastModified']/1000) > upto_date } break end

And then remove the multiple returns at the bottom

haha cool. I originally had something similar to this, but then I thought I should keep it a one-liner (looked at a couple of other break statements). Changed it to like you suggested!
7c572e2

joshdholtz · 2018-08-08T00:47:53Z

spaceship/lib/spaceship/tunes/tunes_client.rb

+
+      return all_reviews if upto_date.nil?
+
+      return all_reviews.select { |review| Time.at(review['value']['lastModified']/1000) > upto_date  }


Remove these multiple returns and just do... (see above comment)

return all_reviews

scorpion35 · 2018-08-08T01:08:22Z

@joshdholtz I will add tests tomorrow and look into fixing the linting as well!

joshdholtz · 2018-08-08T01:22:00Z

@scorpion35 Sounds great! Just ping me when its ready for a review again 😊

Branch: "reviews_uptodate" | Commit Type: "Bug fix" | Changes tested: "no"

scorpion35 · 2018-08-13T13:55:38Z

I signed it!

googlebot · 2018-08-13T13:55:42Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

scorpion35 · 2018-08-13T14:09:19Z

I signed it!

googlebot · 2018-08-13T14:09:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Branch: "reviews_uptodate" | Commit Type: "New test" | Changes tested: "no"

Branch: "reviews_uptodate" | Commit Type: "Minor" | Changes tested: "yes"

This reverts commit ee69cfe.

This reverts commit c57e713.

scorpion35 · 2018-08-13T17:35:34Z

@joshdholtz Fixed the linting test. I have also signed the CLA.

I tried to write new tests, but couldn't seem to figure it out. c57e713 & ee69cfe

Could you give me some guidance on how to make a new test, please? I have tried to place a binding.pry in the new test, but the test seems to be stuck forever; not sure what I am doing wrong. Any help is greatly appreciated!

joshdholtz · 2018-08-14T13:12:57Z

@scorpion35 Yup, I can help! I'm not sure if I'll have time today but I will look tomorrow/Thursday. Are you cool if I push commits to this PR with tests?

scorpion35 · 2018-08-14T14:24:15Z

@joshdholtz Yes, please! Thank you very much :)

I have also signed the CLA (using my both accounts). It seems an admin need to manually set it to yes.

scorpion35 · 2018-08-24T09:50:41Z

@joshdholtz Any update, please? :)

scorpion35 · 2018-08-28T15:21:44Z

@joshdholtz Any help, please? :) Thanks in advance!

scorpion35 · 2018-08-30T14:47:47Z

@joshdholtz @taquitos @KrauseFx Kindly let me know how to add tests to Fastlane, please. I tried to add them but wasn't able to figure it out. If you can give me some tips on how to add them, I will try again.

I am trying to move my script to a remote machine but would like to avoid configuring the remote machine to use my custom changes, would prefer to have the machine automatically update to latest version of Fastlane available! :)

taquitos

Lint it up, then everything looks great!
🎈:goat:

joshdholtz · 2018-08-30T16:35:59Z

@scorpion35 Merged! Thanks for making this changing 😊

scorpion35 · 2018-08-30T16:41:33Z

You are the best, thank you very much @joshdholtz :)

It seems I over thought about these tests, almost had it (ee69cfe), but over complicated it with some other changes :D haha.

Thanks again! 🙇 Do you know when can I expect these changes in official Fastlane gem?

fastlane-bot · 2018-09-04T19:32:45Z

Congratulations! 🎉 This was released as part of fastlane 2.103.0 🚀

* Limit reviews upto date specified. * Optimizations. Implemented suggestions from comments. * Removed debug line * Optimizations and simplified the code so it's easy to read. * Fixed linting Branch: "reviews_uptodate" | Commit Type: "Bug fix" | Changes tested: "no" * Test for reviews upto_date Branch: "reviews_uptodate" | Commit Type: "New test" | Changes tested: "no" * Bug fix in upto date test Branch: "reviews_uptodate" | Commit Type: "Minor" | Changes tested: "yes" * Revert "Bug fix in upto date test" This reverts commit ee69cfe. * Revert "Test for reviews upto_date" This reverts commit c57e713. * Added tests for new upto_date parameter

Limit reviews upto date specified.

6604ea3

googlebot added the cla: no label Aug 6, 2018

scorpion35 mentioned this pull request Aug 6, 2018

Reviews takes very long time. #13030

Closed

joshdholtz requested changes Aug 7, 2018

View reviewed changes

Prasad Anne added 2 commits August 7, 2018 19:42

Optimizations. Implemented suggestions from comments.

e846dd1

Removed debug line

7f0ec0f

joshdholtz requested changes Aug 8, 2018

View reviewed changes

Optimizations and simplified the code so it's easy to read.

7c572e2

Fixed linting

4620254

Branch: "reviews_uptodate" | Commit Type: "Bug fix" | Changes tested: "no"

Prasad Anne added 4 commits August 13, 2018 11:10

Test for reviews upto_date

c57e713

Branch: "reviews_uptodate" | Commit Type: "New test" | Changes tested: "no"

Bug fix in upto date test

ee69cfe

Branch: "reviews_uptodate" | Commit Type: "Minor" | Changes tested: "yes"

Revert "Bug fix in upto date test"

ebf276b

This reverts commit ee69cfe.

Revert "Test for reviews upto_date"

5854b2b

This reverts commit c57e713.

fastlane-bot added the status: needs-attention label Aug 20, 2018

scorpion35 closed this Aug 28, 2018

scorpion35 reopened this Aug 28, 2018

taquitos approved these changes Aug 30, 2018

View reviewed changes

Added tests for new upto_date parameter

8582bc9

joshdholtz force-pushed the reviews_uptodate branch from 2b014cb to 8582bc9 Compare August 30, 2018 16:28

joshdholtz approved these changes Aug 30, 2018

View reviewed changes

joshdholtz merged commit 413fb07 into fastlane:master Aug 30, 2018

scorpion35 deleted the reviews_uptodate branch August 30, 2018 16:43

fastlane-bot removed the status: needs-attention label Sep 1, 2018

joshdholtz mentioned this pull request Sep 4, 2018

Version bump to 2.103.0 #13278

Merged

fastlane-bot added the status: released label Sep 4, 2018

fastlane locked and limited conversation to collaborators Nov 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit reviews upto date specified. #13040

Limit reviews upto date specified. #13040

scorpion35 commented Aug 6, 2018

googlebot commented Aug 6, 2018

joshdholtz left a comment

joshdholtz Aug 7, 2018

scorpion35 Aug 7, 2018

joshdholtz Aug 7, 2018

scorpion35 Aug 7, 2018

joshdholtz Aug 7, 2018

scorpion35 Aug 7, 2018

joshdholtz Aug 7, 2018

scorpion35 Aug 7, 2018

scorpion35 commented Aug 7, 2018

joshdholtz left a comment •

edited

joshdholtz Aug 8, 2018

scorpion35 Aug 8, 2018

joshdholtz Aug 8, 2018

scorpion35 Aug 8, 2018

scorpion35 commented Aug 8, 2018

joshdholtz commented Aug 8, 2018

scorpion35 commented Aug 13, 2018

googlebot commented Aug 13, 2018

scorpion35 commented Aug 13, 2018

googlebot commented Aug 13, 2018

scorpion35 commented Aug 13, 2018 •

edited

joshdholtz commented Aug 14, 2018

scorpion35 commented Aug 14, 2018

scorpion35 commented Aug 24, 2018

scorpion35 commented Aug 28, 2018

scorpion35 commented Aug 30, 2018

taquitos left a comment

joshdholtz commented Aug 30, 2018

scorpion35 commented Aug 30, 2018

fastlane-bot commented Sep 4, 2018


		return all_reviews if upto_date.nil?

		return all_reviews.select { \|review\| Time.at(review['value']['lastModified']/1000) > upto_date }

Limit reviews upto date specified. #13040

Limit reviews upto date specified. #13040

Conversation

scorpion35 commented Aug 6, 2018

googlebot commented Aug 6, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

joshdholtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorpion35 commented Aug 7, 2018

joshdholtz left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorpion35 commented Aug 8, 2018

joshdholtz commented Aug 8, 2018

scorpion35 commented Aug 13, 2018

googlebot commented Aug 13, 2018

scorpion35 commented Aug 13, 2018

googlebot commented Aug 13, 2018

scorpion35 commented Aug 13, 2018 • edited

joshdholtz commented Aug 14, 2018

scorpion35 commented Aug 14, 2018

scorpion35 commented Aug 24, 2018

scorpion35 commented Aug 28, 2018

scorpion35 commented Aug 30, 2018

taquitos left a comment

Choose a reason for hiding this comment

joshdholtz commented Aug 30, 2018

scorpion35 commented Aug 30, 2018

fastlane-bot commented Sep 4, 2018

joshdholtz left a comment •

edited

scorpion35 commented Aug 13, 2018 •

edited