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

try to restructure fcm client #107

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

Roy-Mao
Copy link
Collaborator

@Roy-Mao Roy-Mao commented May 10, 2022

split Fcm class into separate client classes based on the protocols used.

@sabman
Copy link
Member

sabman commented May 10, 2022

@Roy-Mao as much as I appreciate you taking interest in the gem I would like us to make sure that the gem stays simple and easy to maintain. So the level abstraction should not feel overly engineered. Just keep that in mind.

@Roy-Mao
Copy link
Collaborator Author

Roy-Mao commented May 11, 2022

@sabman

Thanks!

I think it would be better if we split FCM client class into two separate classes, supporting api-key and oauth2.0 protocal respectively.

fcm_http_v1_client = Fcm::HttpV1Client.new(json_key_path_credential)
fcm_http_v1_client.send_notification("payload")


fcm_legacy_client = Fcm::Client.new(api_key_credential)
fcm_legacy_client.send_notification("payload")

Can I have write access to this repository so that I can create a new branch for this improvement?

@Roy-Mao Roy-Mao force-pushed the restructure_client branch 3 times, most recently from a1961ea to 9953ec9 Compare May 13, 2022 05:06
@sabman
Copy link
Member

sabman commented May 13, 2022

Can I have write access to this repository so that I can create a new branch for this improvement?

@Roy-Mao let's do a joint review first. Just let me know when this branch is ready to be reviewed and we can maybe even jump on a call and review this refactor together 😄 🚀

@Roy-Mao Roy-Mao force-pushed the restructure_client branch 2 times, most recently from 0885c06 to 3d7cd6e Compare May 16, 2022 02:37
@Roy-Mao
Copy link
Collaborator Author

Roy-Mao commented May 16, 2022

Can I have write access to this repository so that I can create a new branch for this improvement?

@Roy-Mao let's do a joint review first. Just let me know when this branch is ready to be reviewed and we can maybe even jump on a call and review this refactor together 😄 🚀

sure! I am working on it. Will let you know once it is ready for review

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

undefined method `map!' for "spec/*.rb":String
undefined method `map!' for "spec/*.rb":String
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:296:in `block in make_excludes_absolute'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:268:in `each_key'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:292:in `make_excludes_absolute'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:239:in `check'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:231:in `create'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:54:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

undefined method `map!' for "spec/*.rb":String
undefined method `map!' for "spec/*.rb":String
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:296:in `block in make_excludes_absolute'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:268:in `each_key'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:292:in `make_excludes_absolute'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:239:in `check'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config.rb:231:in `create'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:54:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

@Roy-Mao Roy-Mao marked this pull request as ready for review May 19, 2022 03:46
@Roy-Mao Roy-Mao marked this pull request as draft May 19, 2022 03:47
@Roy-Mao Roy-Mao changed the title try to re-structure fcm client try to restructure fcm client May 19, 2022
@Roy-Mao
Copy link
Collaborator Author

Roy-Mao commented May 20, 2022

This PR aims to share with you a general idea of where to improve in the gem and how can we improve it.

Where to improve

  • class design and directory structure not clear
    • everything in one file(class) lib/fcm.rb, including:
      • faraday http agent config and setup
      • api connections based on http_v1 protocol
      • api connections based on http_legacy protocol
      • google oauth authentication
      • response construction
  • coding style
    • existed code base violates the coding style set by hound
    • documentation style not specified
  • lack of error handling and retry implementation for lost connection
  • lack of deprecation warnings
    • Firebase reference
    • Firebase suggests that Apps using the FCM legacy HTTP API should consider migrating to the HTTP v1 API
  • test
    • #manage_topic_relationship is not tested, and the covert test coverage is 89.4% reported by simplecov
    • manually stub http request a lot, maybe vcr gem is a better option

PR content

  • a more clear file structure and class design

    • lib/fcm/client.rb => Fcm::Client.new(api_key).any_legacy_api
    • lib/fcm/v1_client.rb => Fcm::ClientV1.new(json_key_path).any_v1_api
    • lib/fcm/client/*.rb => categorized by the type of legacy protocol api
    • lib/fcm/client_v1/*.rb => categorized by the type of v1 protocol api
    • lib/connection.rb => where to setup faraday agent for http connection
      • use typhoeus as adapter
      • retry on http connection error, such as timeout
    • lib/response.rb => where to build customized gem response based on faraday response
    • lib/error.rb => where to build customized gem error
  • fix rubocop(hound) violations of existing code

    • add rubocop and rubocop-rspec gem
    • overwrite som of the rules in .rubocop.yml
  • integrate simplecov for test coverage and yard for documentation in dev mode

Future improvements

  • improve the test coverage to 90+ would be a bonus
  • using vcr for external http connection to firebase
  • integrate simplecov to CircileCi work flow
  • more robust retry mechanism with exponential backoff

I have no intention of merging it to the master branch right now, which is very risky.
Instead I wanna have a discussion with you based on the facts I listed above over the phone first.
Then we can take a gradual approach to improve the gem by:

  1. make a decision about coding style
  2. know how we are gonna refactor the gem and complete test cases
    a. write tests for every public methods
    b. integrate simplecov into CircleCI workflow
  3. migrate old deprecated method to new method gradually

@sabman
Let me how you think and maybe we can have casual call over the phone 😄 🚀

@sabman
Copy link
Member

sabman commented May 20, 2022

@Roy-Mao yes that would be awesome let's do a call. Can you send me meeting request for later next week via https://calendly.com/sabman and we can do a paired code review

module Fcm
# underhood http client using faraday with typhoeus adapter
module Connection
DEFAULT_TIMEOUT = 30
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

todo:
its better to overwrite the value.

@chrisedington
Copy link

Just spent some time being confused why the V1 didnt work on topic subscriptions -- I think this merge would help alot of people.

@sabman
Copy link
Member

sabman commented Oct 22, 2022

@chrisedington I was waiting for @Roy-Mao to add tests. But seems he's busy now. I'll take a look next week.

@Roy-Mao
Copy link
Collaborator Author

Roy-Mao commented Oct 24, 2022

@chrisedington
@sabman

Sorry for the late reply, I am very busy recently. It would be very helpful if someone can help me add some tests for all the public methods. I will look back into this PR weekend!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants