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

Add better reply for unknown action and subscriptions (alternative implementation) #28

Merged
merged 1 commit into from Nov 1, 2018

Conversation

@moofkit
Copy link
Contributor

@moofkit moofkit commented Oct 28, 2018

It is alternative implementation of solving #7
There is a problem with @NikitaNaumenko approach that PolicyCaller is now knowing to much about streams. And reason that we need some how to combine meta information and error reason and render it to the stream.
So I decide to save some meta information in exception itself and dig it in place where now errors handles #12 (comment). Thx @wilddima for idea.

@coveralls
Copy link

@coveralls coveralls commented Oct 28, 2018

Pull Request Test Coverage Report for Build 137

  • 49 of 49 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 99.024%

Totals Coverage Status
Change from base Build 124: 0.08%
Covered Lines: 406
Relevant Lines: 410

💛 - Coveralls

Copy link
Collaborator

@dsalahutdinov dsalahutdinov left a comment

Hey, @moofkit ! Thank you for the pull request. Nice job, looks good!
I have some small improvements remarks, could you please see them.


expect(described_class.new(exception).message).to eq(['unknownPolicy', 123])
end
end
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Oct 28, 2018

Please add unknown error test case

end
end

def action_type
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Oct 28, 2018

What do you think about adding the methods?:

def subscribe?
  action.type == 'logux/subscribe'
end

def action?
   action.type != 'logux/subscribe'
end

Adding them would make the code more clear:

raise Logux::NoActionError.new(message, meta: meta) if action?

# frozen_string_literal: true

module Logux
class ErrorHandler
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Oct 28, 2018

wdyt about renaming this class to ErrorRenderer or something like this to better match its purpose?

when Logux::NoPolicyError
['unknownPolicy', exception.meta.id]
else
[:error].to_json
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Oct 28, 2018

i think it would be better to keep the answer format unified for every error we have and render the same schema's json:

[exeption.class.camelize(:lower), exception.meta.id]

Copy link
Contributor Author

@moofkit moofkit Oct 28, 2018

@ai I have changed a little format of error messages to unify format. Is it good for you?

Copy link
Member

@ai ai Oct 28, 2018

It is OK, but there is no such think as unknownActionError. It is OK to not have action handler. Only policy is required. This is why we need only unknownAction error (remove current unknownAction and rename unkownPolice to unknownAction).

Also we don’t need Error suffix. Unknown means always an error. Let’s make them shorter.

Copy link
Contributor Author

@moofkit moofkit Oct 29, 2018

I am a little confused. So as I understand correct:

  • return unknownAction if verify_authorized=true and there are NO POLICY
  • just literally do nothing if verify_authorized=true and there are POLICY but ACTION is missing
  • do nothing if verify_authorized=false and there are no ACTION
  • drop the Error suffix

Copy link
Member

@ai ai Oct 29, 2018

Yeap 👍

@moofkit moofkit force-pushed the fix/uknown_action branch 2 times, most recently from 188a101 to 9674fb4 Oct 29, 2018
gazay
gazay approved these changes Oct 29, 2018
Copy link
Member

@gazay gazay left a comment

👍

@moofkit
Copy link
Contributor Author

@moofkit moofkit commented Oct 30, 2018

⚠️ It is not ready for merge ⚠️
#28 (comment)
I will fix this as soon as possible

@moofkit moofkit changed the title Add better reply for unknown action and subscriptions (alternative implemantation) Add better reply for unknown action and subscriptions (alternative implementation) Oct 31, 2018
@moofkit
Copy link
Contributor Author

@moofkit moofkit commented Oct 31, 2018

Now it's ready for review 🎊

lib/logux.rb Outdated
end
end

UnknownPolicyError = Class.new(WithMetaError)
Copy link
Contributor Author

@moofkit moofkit Oct 31, 2018

we don't need this error anymore. should be deleted

@ai
Copy link
Member

@ai ai commented Oct 31, 2018

Seems good from Logux API perspective. @wilddima @dsalahutdinov is Ruby code style passes our requirements?


private

def build_message(exception, additional_info = nil)
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Nov 1, 2018

additional_info parameter default value is not used, that's why compact method call ⬇️ is also not needed

@@ -87,4 +89,109 @@
expect { request_logux }.to change { logux_store.size }.by(1)
end
end

# rubocop: disable RSpec/NestedGroups
# TODO: refactoring; may be move this cases to separated file
Copy link
Collaborator

@dsalahutdinov dsalahutdinov Nov 1, 2018

@moofkit , what do you think about doing it with the next pull request? 😉

Copy link
Contributor Author

@moofkit moofkit Nov 1, 2018

@dsalahutdinov, yeah of course 👍

@moofkit
Copy link
Contributor Author

@moofkit moofkit commented Nov 1, 2018

Ok, I will polish this, rebase, squash and it is ready to go! 🚀

@moofkit moofkit force-pushed the fix/uknown_action branch from b71896d to 6145262 Nov 1, 2018
@moofkit
Copy link
Contributor Author

@moofkit moofkit commented Nov 1, 2018

Ok, I have done here!

@dsalahutdinov dsalahutdinov merged commit fec57b2 into logux:master Nov 1, 2018
2 checks passed
@moofkit moofkit deleted the fix/uknown_action branch Nov 1, 2018
araslanov-e
Copy link

araslanov-e commented on 6145262 Nov 8, 2018

Why no more conversion to json? to_json in Logux::Stream

dsalahutdinov
Copy link
Collaborator

dsalahutdinov commented on 6145262 Nov 9, 2018

hi, @araslanov-e ,it looks like a mistake

araslanov-e
Copy link

araslanov-e commented on 6145262 Nov 9, 2018

@dsalahutdinov no no. to_json in Logux::Stream

dsalahutdinov
Copy link
Collaborator

dsalahutdinov commented on 6145262 Nov 9, 2018

@araslanov-e really, it is

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

Successfully merging this pull request may close these issues.

None yet

6 participants