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

Adding resending functions to the action controller

Merged
merged 6 commits into from Aug 24, 2020
Merged

Adding resending functions to the action controller #8

merged 6 commits into from Aug 24, 2020

Conversation

privorotskii
Copy link
Contributor

@privorotskii privorotskii commented Apr 5, 2020

Here is the draft of PR for this issue.

I tried to add a possibility of performing resend as soon, as ActionController#resend is called. But there is a little difficulty - for implementing it, we need to somehow access the stream from the ActionController, or we need to call the method of some object that has access to the stream.

The only classes, which have access to the stream - are Process namespace classes (except app.rb itself). I considered two options:

  1. Pass the stream from Process::Action to the ActionCaller's initialization and then to the controller's initialization. So, we can write to the stream directly from the ActionCaller#resend method.
  2. Use exactly the same way for passing, but pass a lambda instead of the stream itself

The first approach is not very architecturally good, is it? I don't think that controller should have access to the stream directly. So, I implemented the second one. I added nil defaults to the classes initialize methods so as not to change a lot of specs (that's why it is a draft, not a PR yet).

Is this approach acceptable? If so, I would fix specs so as not to pass default nil and polish code a bit. If not, do you have any advice, how to implement it better?

@ai
Copy link
Member

ai commented Apr 5, 2020

@dsalahutdinov seems like we need to change architecture a little for resend and need your advice here

@ai
Copy link
Member

ai commented Apr 5, 2020

@TolichP sorry, I made a mistake in origin task.

We need to send resend before approved. Action will send to other clients immidently after approved message, so we need to already have list or receivers.

@privorotskii
Copy link
Contributor Author

privorotskii commented Apr 9, 2020

@ai, okay, I got your idea.
So, if we need to send resend before authorization performing and, therefore, before action processing - we apparently cannot call resend from the action. So, we should define a list of receivers in another place, right? The only idea I have is to do it like this:

module Actions
  class Comment < Logux::ActionController

    # define receivers here, specifying action name
    resend_receivers :add, channel: 'users'

    # if we need any data from Logux::BaseController's @meta, use lambda:
    resend_receivers :mark_as_approved, ->(meta) { do_something(meta) }

    # no manual `resend` calls in actions
    def add
      ...
      respond :processed
    end

    def mark_as_approved
      ...
      respond :processed
    end
  end
end

module Logux
  class ActionController < Logux::BaseController
    ...
    # DSL-like method for using in, for example, Comment
    def self.resend_receivers(action_name, receivers)
      # memorize receivers in an instance variable on a class
      return @resend_receivers[action_name] = receivers unless receivers.respond_to?(:call)

      @resend_receivers[action_name] = receivers.call(meta)
    end

    def self.receiver_by_action(action_name)
      @resend_receivers[action_name]
    end
    ...
  end

  module Process
    class Action  
    ...
      def call
        process_resend!
        process_authorization!
        process_action!
      end

      def process_resend!
        # find an action_controller, find action_name
        # call action_controller's receiver_by_action(action_name)
        # and get receivers

        stream.write(['resend', meta_from_chunk['id'], receivers])
        stream.write(',')
      end
    ...
    end
  end
end

Let me know if you have any thoughts about it.

@ai
Copy link
Member

ai commented Apr 9, 2020

@TolichP I like new API

Add such method as DSL for action_controller or children of it
@privorotskii privorotskii marked this pull request as ready for review Apr 11, 2020
@privorotskii privorotskii changed the title WIP: adding #resend method to action controller Adding resending functions to the action controller Apr 11, 2020
@privorotskii
Copy link
Contributor Author

privorotskii commented Apr 11, 2020

@ai Done, can you take a look at this, please?

I have no idea, why Travis is failing, I can't see the error. Tests and rubocop on my machine are passing correctly.

@ai
Copy link
Member

ai commented Apr 11, 2020

The same for me. It could be Travis CI problem.

@ai
Copy link
Member

ai commented Apr 11, 2020

@TolichP resend is more likely to depend on the action. For instance, if you have an action to change the post, you want to send it to the post’s channel.

  resend :edit, ->(action) {
    { channel: "post/#{ action.id }" }
  }

Also resend_receivers have two problems:

  1. It’s long. It hard to memorize (especially, if you are not good in English)
  2. It is not grammar correct. We are declaring whom we want to resend it. resend_to will be better.
  3. But I am suggesting to keep the same names across Logux implementations

Method is renamed to be unified with other logux implementations
and to be shorter and simpler
This spec was intended to check, that calling process_resend! for
controller, that was not accidentally inherited from ActionController
and not having receivers_by_action method - won't fail and will just
return nil. So, the test class was inherited from ChannelController.
But there is a problem - if this test class is inherited from ChannelController,
it leads to channels_spec failing.
The GC cannot remove test class in time and it exists while channels_spec
executing sometimes`. There are extra descendants of ChannelController
and test goes wrong. So I changed spec to simpy remove desired method.
@privorotskii
Copy link
Contributor Author

privorotskii commented Apr 12, 2020

Yes, indeed.
Done, resend_receivers renamed to resend as it is named in node.js implementation, resend lambda uses action instead of meta.

@ai
Copy link
Member

ai commented Apr 12, 2020

/cc @dsalahutdinov

@dsalahutdinov
Copy link
Collaborator

dsalahutdinov commented Apr 17, 2020

Hi! I will review this PR on the next week. Have no time right now. Sorry

@dsalahutdinov
Copy link
Collaborator

dsalahutdinov commented Aug 11, 2020

@dreikanter

@dreikanter dreikanter merged commit ef73fa5 into logux:master Aug 24, 2020
1 check passed
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.

None yet

4 participants