134929249 two flash messages bug #418

OlenaCh · 2016-12-21T21:13:57Z

Fixes: https://www.pivotaltracker.com/story/show/134929249

Fixing bug with an error flash message
Fixing bug with appearance of two flash messages which of them is informing that an organization was successfully approved. Currently, organizations can be approved only if they have valid email and only in this case the abovementioned success message will be shown.
Adding relevant RSpec tests and slightly changing existing Cucumber tests.

johnnymo87 · 2016-12-21T21:44:04Z

app/services/accept_proposed_organisation.rb

-      create_invitation_response_object(InviteUnregisteredUserFromProposedOrg.new(@email,org).run, org)
+    begin
+      accept_organization
+    rescue Exception


Could we rescue a specific error here rather than Exception?

Also, you can remove the begin ... rescue ... end, and just have rescue to spaces to the left. http://seejohncode.com/2012/04/17/short-tip-rescuing-a-method-in-ruby/

johnnymo87 · 2016-12-21T21:46:35Z

app/services/accept_proposed_organisation.rb

+  def accept_organization
+    org = @proposed_org.accept_proposal
+    @result = inform_user org
+    raise Exception if [Response::NOTIFICATION_SENT, 


To raise a domain-specific error, we could do the following:

class NotificationError < StandardError; end def accept_organisation # raise NotificationError if ... end

johnnymo87 · 2016-12-21T21:47:26Z

Please replace all references to organization with organisation 🇬🇧 😉

OlenaCh · 2016-12-22T22:27:48Z

Ok, I'll do this! Thank you very much for the feedback :) 2016-12-21 23:47 GMT+02:00 Jon Mohrbacher <notifications@github.com>:

…

Please replace all references to organization with organisation 🇬🇧 😉 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#418 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AS9L-Omxa7ak16-s-V9F2qDk7QRPUSiaks5rKZ5vgaJpZM4LTYbo> .

OlenaCh · 2016-12-28T01:49:20Z

I've changed code according to the recommendations and Code Climate requirements. Thank you! :)

johnnymo87 · 2016-12-28T10:00:46Z

+1 nice tests

OlenaCh · 2016-12-28T10:16:51Z

Thank you! :) 2016-12-28 12:00 GMT+02:00 Jon Mohrbacher <notifications@github.com>:

…

+1 nice tests — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#418 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AS9L-OCcc3gGEqv_57s5lRYJAo-R6z_Oks5rMjNPgaJpZM4LTYbo> .

Maroo-b

@OlenaCh really good PR just a note about code style in Ruby, avoid adding multiple statement in same line with semicolon.

Maroo-b · 2017-01-06T13:25:37Z

.rvmrc

@@ -1 +0,0 @@
-rvm use 2.3.1@localsupport --create


is there a reason to delete the .rvmrc file ?

Oh, I'm sorry, I did not delete it on purpose. Perhaps, it was deleted accidentally and I did not notice, so did not create again. I'll fix it.

Maroo-b · 2017-01-09T13:14:15Z

app/controllers/proposed_organisations_controller.rb

-    redirect_to organisation_path(result.accepted_organisation) and return false
+    rslt = prepare_data_for_update
+    redirect_to organisation_path(rslt.accepted_org) and return false if rslt.accepted_org
+    redirect_to proposed_organisations_path


what about using if/else block like

if rslt.accepted_org redirect_to organisation_path(rslt.accepted_org) else redirect_to proposed_organisations_path end

Actually, that was the first version of the code; but CodeClimate analysis marked it as an issue of ABC complexity (too much if/else statements for update method). Should I return back to the previous version?

it's not a blocker for my side when I have two alternatives I pick the one that make my code easier to read.

Yes, of course, I understand. I am sorry, but may I ask about clarification, please? It will add two additional CC issues (6 instead of 5 allowed lines of code in one method & ABC complexity). It is not a problem to change these lines, but one of first comments on my PR was about necessity to address CodeClimate issues (when we discussed it on Slack with @tansaku).

@OlenaCh that's strange because I saw only the redirection condition on update method, you can keep the current implementation.
Generally when it comes to CodeClimate warnings, it's always better to address those warnings but sometimes you can ignore it but you need confirmation from reviewers :)

Ok, thank you very much :) !

Maroo-b · 2017-01-09T13:47:39Z

app/services/accept_proposed_organisation.rb

+
+  def accept_organisation
+    org = @proposed_org.accept_proposal; @result = inform_user org
+    raise NotificationError if success?(@result.status) == false


when it comes to predicate method (ends with ? like success?) you don't need to compare with boolean for example we can change the expression to:

raise NotificationError unless success?(@result.status)

Usually I avoid using exception for managing the flow, it's ok to use it but as alternative I would check the status of inform_user before accepting the organisation.

Thank you! I'll fix it! :)

Maroo-b · 2017-01-09T22:16:58Z

app/models/proposed_organisation.rb

-    org = becomes!(Organisation)
+
+  def accept_proposal(rollback = false)
+    org = rollback ? becomes!(ProposedOrganisation) : becomes!(Organisation)


I understand that there is change from Organisation to ProposedOrganisation it will be better if you add a method to Organisation instead of adding a control flag (it's called control coupling )

Ok, I'll do this. Thank you!

OlenaCh · 2017-01-14T23:01:20Z

@johnnymo87 @Maroo-b looks like the test coverage slightly dropped :(. It appears that there is one line in AcceptProposedOrganisation service (OTHER_ERROR) which currently I have no idea how to hit. Anyway I'll dig deeper into it during the following week.

Maroo-b · 2017-01-17T08:09:35Z

@OlenaCh usually when writing unit test for a service, we usually prefer not depending on the result of outside this service's (class) methods.
In our case, to test the output for Response::OTHER_ERROR response, the code depends on the result of

InviteUnregisteredUserFromProposedOrg.new(@email, @org).run

So I suggest to stub that method using the following syntax before the expectation:

#create an object that responds to method status,
# other_response.status == Response::OTHER_FAILURE
other_response = double(status: Response::OTHER_FAILURE)

#Now we tell the InviteUnregisteredUserFromProposedOrg to return the other_response object
#InviteUnregisteredUserFromProposedOrg.new(arg1,arg2).run == other_response
#It's possible to use this alternative syntax also:
#allow_any_instance_of(InviteUnregisteredUserFromProposedOrg).to receive(:run).and_return(other_response)
allow(InviteUnregisteredUserFromProposedOrg.to receive_message_chain(:new, :run).and_return(other_response)

You can read more about stub from RSpec docs: https://relishapp.com/rspec/rspec-mocks/v/3-5/docs/basics

By the way @johnnymo87 do you think that we should inject the service: InviteUnregisteredUserFromProposedOrg into the AcceptProposedOrganisation ?

OlenaCh · 2017-01-17T13:17:39Z

@Maroo-b thank you for a tip! :)

tansaku · 2017-01-18T18:01:10Z

@OlenaCh we've got a few merge conflicts here now - let me know if you have trouble resolving them

OlenaCh · 2017-01-19T21:29:16Z

@tansaku I've resolved the conflict and committed changes. I'll look for a bug and then commit a fix for it. Thank you!

Maroo-b · 2017-01-23T13:01:05Z

@OlenaCh there is a failing test, does it pass locally ?

OlenaCh · 2017-01-23T23:52:40Z

@Maroo-b, it also fails locally. It appeared after merging branches, but looks like I've fixed it today :).

tansaku · 2017-01-25T16:43:05Z

@OlenaCh great work getting this green!

BTW, we leave the PT tickets in the started state till thinks are actually merged. With the right commit messages the merging of the PR will automatically mark the PT ticket as finished. See https://github.com/AgileVentures/LocalSupport/blob/develop/CONTRIBUTING.md for more details. Even if you don't have the git commits set up to hook into tracker, I can click "Finish" when I merge.

I hope to review and merge soon - I've just got to give priority to premium PRs (over several projects including this one) in the first instance http://www.agileventures.org/pricing so sometimes I get a little backed up :-)

OlenaCh · 2017-01-25T21:40:47Z

@tansaku oh, sorry for inconvenience! In the future I'll keep about tickets in mind. Thank you! I'm awaiting for your review and feedback; simultaneously, I'm going to choose a new ticket to do. When this one will be merged, I'll be able to proceed to the next task :)

tansaku · 2017-01-26T11:15:14Z

@johnnymo87 @Maroo-b - if you're all happy with this one I can give it a final review

tansaku · 2017-02-02T15:56:50Z

@Maroo-b @johnnymo87 any outstanding issues on this one? I'd like to get it merged in, but am struggling to find time to do the detailed review I'd like to

johnnymo87 · 2017-02-02T16:18:53Z

Nope, my original +1 still stands.

Maroo-b · 2017-02-14T07:44:23Z

+1

tansaku · 2017-02-21T11:24:03Z

great work @OlenaCh - sorry for delay - I finally got to look through in detail and looks like a big improvement - really appreciate your hard work

OlenaCh · 2017-02-23T19:20:17Z

@tansaku thank you very much for the feedback - I am very glad that you liked the PR! Looks like I can proceed with new issues! :)

* Fixing error flash bug * Without valid email organization is not accepted * Cucumber fix * Solving Code Climate issues - pull AgileVentures#418 * Refactoring flash messages into a service * Fixing Code Climate issues * Fixing Code Climate issues 2.0 * Adding new method to Organisation model * CodeClimate new issue fix * One more fix for CC issues * One more fix for CC issues 2.0 * One more fix for CC issues 3.0 * RSpec refactoring * More RSpec tests for AcceptProposedOrganisation service * Cucumber fix

Olena_Ch added 4 commits December 11, 2016 23:50

Fixing error flash bug

7251c6d

Without valid email organization is not accepted

c2bb75a

Merge branch 'master' into develop

e717715

Cucumber fix

19f1437

johnnymo87 reviewed Dec 21, 2016

View reviewed changes

Olena_Ch added 4 commits December 26, 2016 23:28

Solving Code Climate issues - pull AgileVentures#418

fb2ad2b

Refactoring flash messages into a service

e8aa731

Fixing Code Climate issues

e587ffc

Fixing Code Climate issues 2.0

c938f70

Maroo-b reviewed Jan 9, 2017

View reviewed changes

Olena_Ch added 6 commits January 14, 2017 00:40

Adding new method to Organisation model

ed7850d

CodeClimate new issue fix

83f1258

One more fix for CC issues

06a7796

One more fix for CC issues 2.0

201bff8

One more fix for CC issues 3.0

7815f11

RSpec refactoring

f182fab

More RSpec tests for AcceptProposedOrganisation service

150e110

Merge branch 'develop' into 134929249_two_flash_messages_bug

70697cb

Cucumber fix

d29ca55

Merge branch 'develop' into 134929249_two_flash_messages_bug

d94f37e

tansaku merged commit 9f60a38 into AgileVentures:develop Feb 21, 2017

134929249 two flash messages bug #418

134929249 two flash messages bug #418

Conversation

OlenaCh commented Dec 21, 2016 • edited by tansaku Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnymo87 commented Dec 21, 2016

OlenaCh commented Dec 22, 2016 via email

OlenaCh commented Dec 28, 2016

johnnymo87 commented Dec 28, 2016

OlenaCh commented Dec 28, 2016 via email

Maroo-b 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

OlenaCh Jan 10, 2017 • edited Loading

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

OlenaCh commented Jan 14, 2017 • edited Loading

Maroo-b commented Jan 17, 2017

OlenaCh commented Jan 17, 2017

tansaku commented Jan 18, 2017

OlenaCh commented Jan 19, 2017 • edited Loading

Maroo-b commented Jan 23, 2017

OlenaCh commented Jan 23, 2017

tansaku commented Jan 25, 2017

OlenaCh commented Jan 25, 2017

tansaku commented Jan 26, 2017

tansaku commented Feb 2, 2017

johnnymo87 commented Feb 2, 2017

Maroo-b commented Feb 14, 2017

tansaku commented Feb 21, 2017

OlenaCh commented Feb 23, 2017

OlenaCh commented Dec 21, 2016 •

edited by tansaku

Loading

OlenaCh Jan 10, 2017 •

edited

Loading

OlenaCh commented Jan 14, 2017 •

edited

Loading

OlenaCh commented Jan 19, 2017 •

edited

Loading