[spaceship] rework reviews 📢 #8701

hjanuschka · 2017-03-29T11:09:02Z

reworks interface for retreiving app store reviews.

require "spaceship"
Spaceship::Tunes.login("helmut@januschka.com")
app = Spaceship::Application.find("krone.at-fivexfive")


# get all reviews from AT
reviews = app.ratings.reviews("AT")

reviews.each do | review |
  puts "#{review.rating}"
  if review.responded? 
    puts "Already been responed to"
  end
end

googlebot · 2017-03-29T11:09:04Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). 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.

googlebot · 2017-03-29T11:10:31Z

CLAs look good, thanks!

giginet

Great work! 👍
I'm excited at this new feature.

I pointed some issues.
Let's discuss them.

giginet · 2017-03-29T17:04:52Z

spaceship/lib/spaceship/tunes/app_ratings.rb

      def reviews(store_front, versionId = '')
-        client.get_reviews(application.apple_id, application.platform, store_front, versionId)
+        raw_reviews = client.get_reviews(application.apple_id, application.platform, store_front, versionId)
+        reviews = []


I think you can use map.

reviews = raw_reviews.map do |review| review["value"]["application"] = self.application AppReview.factory(review["value"]) end

giginet · 2017-03-29T17:06:45Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      attr_accessor :developer_response
+
+      attr_mapping({
+        'id'       => :id,


I think you don't need to align columns.

attr_mapping({ 'id' => :id, 'rating' => :rating, ....

good catch, was inconsistent too 😢

giginet · 2017-03-29T17:07:26Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+          response_attrs[:application] = obj.application
+          response_attrs[:review_id] = obj.id
+          obj.developer_response = DeveloperResponse.factory(response_attrs)
+          return obj


You can remove return.

sometimes i just forget about the non requirement for explicit return, was stuck in few other languages the last weeks - where return was explicit. -> fixed

ok after testing, removing return here is faulty, and thats exactly the case why i like explicit returns, in this case a DeveloperResponse (which is in a attr of obj) is returned, where as it is intended to return a AppReview (which is hold in obj)

giginet · 2017-03-29T17:13:57Z

spaceship/lib/spaceship/tunes/tunes_client.rb

-      r = request(:get, "ra/apps/#{app_id}/reviews?platform=#{platform}&storefront=#{storefront}&versionId=#{versionId}")
-      parse_response(r, 'data')['reviews']
+      index = 0
+      per_page = 100 # apple default


It should be constants.

this would mean a constant of TunesClient? - i dont like that.

giginet · 2017-03-29T17:18:54Z

spaceship/lib/spaceship/tunes/tunes_client.rb

-      parse_response(r, 'data')['reviews']
+      index = 0
+      per_page = 100 # apple default
+      all_fetched = false


You can get rid of this variable to reduce side-effects.
It is better to break when the loop condition is filled.

changed to loop do

giginet · 2017-03-29T17:37:55Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+
+      def response?
+        return true if raw_developer_response
+        return false


You can remove return.

giginet · 2017-03-29T17:42:38Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+        end
+      end
+
+      def response?


Do you mean this method?

If you mean this app review is already responded from developers.
I think it is better responded?

good point, changed

giginet · 2017-03-29T17:43:16Z

spaceship/lib/spaceship/tunes/tunes_client.rb

+      all_fetched = false
+      all_reviews = []
+      until all_fetched
+        r = request(:get, "ra/apps/#{app_id}/platforms/#{platform}/reviews?storefront=#{storefront}&versionId=#{versionId}&index=#{index}")


Is this OK these requests are executed synchronously?

Isn't it needed delay between each request?

they are synchronously, i tested it on a quiet large app - with a few hundreds of reviews and i haven't had a issue - the web interface also pages one after another.

giginet · 2017-03-29T17:46:53Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+        client.create_developer_response(app_id: application.apple_id, platform: application.platform, review_id: review_id, response: text)
+      end
+
+      def update(text)


I think it should also have bang suffix. because, this change is destructive.

update!

good point, fixed

giginet · 2017-03-29T17:49:57Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -323,9 +323,46 @@ def get_rating_summary(app_id, platform, versionId = '')
      parse_response(r, 'data')
    end

+    def create_developer_response(app_id: nil, platform: nil, review_id: nil, response: "")
+      data = {
+        responseText: response,


[Question] If responseText is default value (empty string).
will API pass the value?

i bet it will fail 👍

hjanuschka

@giginet thx for your time, i addressed almost all of the feedback.

hjanuschka · 2017-03-29T18:27:57Z

spaceship/lib/spaceship/tunes/app_ratings.rb

      def reviews(store_front, versionId = '')
-        client.get_reviews(application.apple_id, application.platform, store_front, versionId)
+        raw_reviews = client.get_reviews(application.apple_id, application.platform, store_front, versionId)
+        reviews = []


hjanuschka · 2017-03-29T18:28:56Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      attr_reader :last_modified
+      attr_reader :hidden
+      attr_reader :state
+      attr_accessor :application


it simply didn't work with _reader if you look at the other spaceship classes, almost all of the attrs are by _accessor

hjanuschka · 2017-03-29T18:29:48Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      attr_accessor :review_id
+
+      attr_mapping({
+        'responseId' => :id,


heard about that, but i tried to stick to the current coding style used across the codebase - and attr_mapping always has the => (e.g: application.rb)

hjanuschka · 2017-03-29T18:31:51Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+        client.create_developer_response(app_id: application.apple_id, platform: application.platform, review_id: review_id, response: text)
+      end
+
+      def update(text)


good point, fixed

hjanuschka · 2017-03-29T18:32:36Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      attr_accessor :developer_response
+
+      attr_mapping({
+        'id'       => :id,


good catch, was inconsistent too 😢

hjanuschka · 2017-03-29T19:06:21Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -323,9 +323,46 @@ def get_rating_summary(app_id, platform, versionId = '')
      parse_response(r, 'data')
    end

+    def create_developer_response(app_id: nil, platform: nil, review_id: nil, response: "")
+      data = {
+        responseText: response,


i bet it will fail 👍

hjanuschka · 2017-03-29T19:06:27Z

spaceship/lib/spaceship/tunes/tunes_client.rb

+      parse_response(r, 'data')
+    end
+
+    def update_developer_response(app_id: nil, platform: "ios", review_id: nil, response_id: nil, response: "")


hjanuschka · 2017-03-29T19:07:34Z

spaceship/lib/spaceship/tunes/tunes_client.rb

-      r = request(:get, "ra/apps/#{app_id}/reviews?platform=#{platform}&storefront=#{storefront}&versionId=#{versionId}")
-      parse_response(r, 'data')['reviews']
+      index = 0
+      per_page = 100 # apple default


this would mean a constant of TunesClient? - i dont like that.

hjanuschka · 2017-03-29T19:10:13Z

spaceship/lib/spaceship/tunes/tunes_client.rb

-      parse_response(r, 'data')['reviews']
+      index = 0
+      per_page = 100 # apple default
+      all_fetched = false


changed to loop do

hjanuschka · 2017-03-29T19:11:23Z

spaceship/lib/spaceship/tunes/tunes_client.rb

+      all_fetched = false
+      all_reviews = []
+      until all_fetched
+        r = request(:get, "ra/apps/#{app_id}/platforms/#{platform}/reviews?storefront=#{storefront}&versionId=#{versionId}&index=#{index}")


they are synchronously, i tested it on a quiet large app - with a few hundreds of reviews and i haven't had a issue - the web interface also pages one after another.

giginet

Thank you to fix them!

I have a few concerns yet.
If they would be fixed, LGTM.

giginet · 2017-03-30T04:08:36Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      end
+
+      def responded?
+        return true if raw_developer_response


If raw_developer_response is nil, responded don't returns false but nil .

I meant dropping return only. 🙇‍♂️

def responded? return true if raw_developer_response false end

omg, thx for that, feeling a bit dumb now, fixed

giginet · 2017-03-30T04:13:10Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -323,9 +323,43 @@ def get_rating_summary(app_id, platform, versionId = '')
      parse_response(r, 'data')
    end

+    def create_developer_response!(app_id: nil, platform: nil, review_id: nil, response: "")


Can app_id, platform and review_id be nil?

According to L332, these seem to be required.
If these values are required, default arguments should not be passed.
They have to be nonnull.

i know something like

def def create_developer_response!(app_id:, platform: , review_id: , response: "")

but it does not work on some older ruby's - i did a similar mistake on the IAP-Implementation, where we reverted to default values.

These values must not be nil.
I think you can drop argument labels.

def create_developer_response!(app_id, platform, review_id, response) end

Are labels needed?

i personally preffer labels, in case of any refactoring its easier to handle.

eg adding a param in the middle.

def create_developer_response!(app_id, platform,new_param, review_id, response)

is sort a nightmare to refactor without labels, with labels it is no big deal.

OK, I understood reasons.
Instead, how about adding nil checking?

hjanuschka

feedback addressed.

hjanuschka · 2017-03-30T05:25:05Z

spaceship/lib/spaceship/tunes/app_ratings.rb

+      end
+
+      def responded?
+        return true if raw_developer_response


omg, thx for that, feeling a bit dumb now, fixed

hjanuschka · 2017-03-30T05:29:00Z

spaceship/lib/spaceship/tunes/tunes_client.rb

@@ -323,9 +323,43 @@ def get_rating_summary(app_id, platform, versionId = '')
      parse_response(r, 'data')
    end

+    def create_developer_response!(app_id: nil, platform: nil, review_id: nil, response: "")


i know something like

def def create_developer_response!(app_id:, platform: , review_id: , response: "")

but it does not work on some older ruby's - i did a similar mistake on the IAP-Implementation, where we reverted to default values.

hjanuschka

@giginet i addressed the handling of the nil params, let me know if this is ready to be merged now 👍

giginet

🚀

hjanuschka · 2017-03-30T12:56:33Z

@KrauseFx could you review this too, about the BC break (see pr body)

KrauseFx

Please wait with merging

KrauseFx · 2017-05-01T19:03:46Z

Hey @hjanuschka, sorry for not coming back earlier. I'd be much in favor to merge the fetching the app reviews first. I personally would prefer not to include the replying to reviews part into spaceship itself. Do you think we could separate those 2?

Turns reviews into objects, and adds the ability to retrieve/create/update developer response

hjanuschka · 2017-05-02T05:51:09Z

@KrauseFx write ops - have been removed.

KrauseFx

Wonderful 🎷

nafu · 2017-05-02T12:18:16Z

Rock! 🚀

hjanuschka · 2017-05-02T12:42:09Z

and roll 🎶

fastlane-bot · 2017-05-02T13:00:12Z

Hey @hjanuschka 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

fastlane-bot · 2017-05-04T13:01:04Z

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

googlebot added the cla: no label Mar 29, 2017

hjanuschka force-pushed the itc_comments branch from 543fdd6 to ccc887f Compare March 29, 2017 11:10

googlebot added cla: yes and removed cla: no labels Mar 29, 2017

hjanuschka changed the title ~~[WIP] [spaceship] rework reviews + support for developer response~~ [spaceship] rework reviews + support for developer response 📢 Mar 29, 2017

giginet requested changes Mar 29, 2017

View reviewed changes

hjanuschka commented Mar 29, 2017

View reviewed changes

giginet approved these changes Mar 30, 2017

View reviewed changes

hjanuschka commented Mar 30, 2017

View reviewed changes

giginet approved these changes Mar 30, 2017

View reviewed changes

hjanuschka requested a review from KrauseFx March 30, 2017 12:55

KrauseFx requested changes Mar 30, 2017

View reviewed changes

fastlane-bot added the status: needs-attention label Apr 12, 2017

hjanuschka changed the title ~~[spaceship] rework reviews + support for developer response 📢~~ [spaceship] rework reviews 📢 May 2, 2017

hjanuschka added 9 commits May 2, 2017 07:50

rework reviews + support for dev response

f35422a

Turns reviews into objects, and adds the ability to retrieve/create/update developer response

fix existing tests

0df0f04

specs for comments

7b26333

adress feedback

cc301ad

do not parse response as it is not in the default way

70241cc

remove all_fetched

cecce8d

fix rubocop issues, and return false if not responded?

71c5952

handle nil values

6f8e67f

remove write ops for dev. response

39c1f27

hjanuschka force-pushed the itc_comments branch from ffffd75 to 39c1f27 Compare May 2, 2017 05:50

hjanuschka mentioned this pull request May 2, 2017

[spaceship] add methods to create/edit developer response #9036

Closed

KrauseFx approved these changes May 2, 2017

View reviewed changes

hjanuschka merged commit 5e180b3 into fastlane:master May 2, 2017

fastlane-bot removed the status: needs-attention label May 2, 2017

fastlane-bot added the status: included-in-next-release label May 2, 2017

mpirri mentioned this pull request May 3, 2017

Version bump to 2.28.8 #9056

Merged

fastlane-bot added status: released and removed status: included-in-next-release labels May 4, 2017

fastlane locked and limited conversation to collaborators Aug 2, 2017

[spaceship] rework reviews 📢 #8701

[spaceship] rework reviews 📢 #8701

Conversation

hjanuschka commented Mar 29, 2017 • edited

googlebot commented Mar 29, 2017

googlebot commented Mar 29, 2017

giginet left a comment

Choose a reason for hiding this comment

giginet Mar 29, 2017 • 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

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

Choose a reason for hiding this comment

giginet Mar 29, 2017 • 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

Choose a reason for hiding this comment

hjanuschka 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giginet 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

giginet Mar 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanuschka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanuschka left a comment

Choose a reason for hiding this comment

giginet left a comment

Choose a reason for hiding this comment

hjanuschka commented Mar 30, 2017

KrauseFx left a comment

Choose a reason for hiding this comment

KrauseFx commented May 1, 2017

hjanuschka commented May 2, 2017

KrauseFx left a comment

Choose a reason for hiding this comment

nafu commented May 2, 2017

hjanuschka commented May 2, 2017

fastlane-bot commented May 2, 2017

fastlane-bot commented May 4, 2017

hjanuschka commented Mar 29, 2017 •

edited

giginet Mar 29, 2017 •

edited

giginet Mar 29, 2017 •

edited

giginet Mar 30, 2017 •

edited