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

134448159 structured data markup breadcrumb #429

Conversation

@cesswairimu
Copy link
Contributor

cesswairimu commented Mar 1, 2017

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 2, 2017

well done getting started on this @cesswairimu

does the build failure and codeclimate comments make sense?

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Mar 8, 2017

@tansaku no it doesn't. I think it might have something to do with the failing cucumber tests but i do not know why they are failing either

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 8, 2017

hmm, odd failure:

  1) Organisation find all orgs that have keyword anywhere in their name or description
     Failure/Error: expect(Organisation.search_by_keyword('elderly')).to eq([@org2, @org3])
     
       expected: [#<Organisation id: 304, name: "Indian Elders Association", address: "64 pinner road", postcode: "HA1...ublish_email: true, deleted_at: nil, type: "Organisation", non_profit: nil, slug: "age-uk-elderly">]
            got: #<ActiveRecord::Relation [#<Organisation id: 305, name: "Age UK Elderly", address: "64 pinner road", ...: true, deleted_at: nil, type: "Organisation", non_profit: nil, slug: "indian-elders-association">]>
     
       (compared using ==)
     
rspec ./spec/models/organisation_spec.rb:206 # Organisation find all orgs that have keyword anywhere in their name or description
@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 8, 2017

it passes locally for me - how about you?

I've restarted the CI build to see if it recurs ...

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 8, 2017

also, what do you think of the codeclimate issues? https://codeclimate.com/github/AgileVentures/LocalSupport/pull/429

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 8, 2017

ah, that rspec fail looks like a random thing (not good, and maybe worthy of investigation later) however now we see some cukes fail:

Failing Scenarios:
cucumber -p second_try features/admin/admin_new_charity.feature:81 # Scenario: Successfully create charity while being signed-in as superadmin from arbitrary page
cucumber -p second_try features/home_page/home_button.feature:27 # Scenario Outline: Find help with care for elderly, Examples (#1)
cucumber -p second_try features/home_page/home_button.feature:28 

and these I can replicate locally, where I get the following message:

  Scenario Outline: Find help with care for elderly         # features/home_page/home_button.feature:20
    Given I visit the <page>                                # features/home_page/home_button.feature:21
    Then I should see an <status> home button in the header # features/home_page/home_button.feature:22
    And I click "Home"                                      # features/home_page/home_button.feature:23
    Then I should be on the home page                       # features/home_page/home_button.feature:24

    Examples: 
      | page                                          | status   |
      | volunteer opportunities page                  | inactive |
      Ambiguous match, found 2 elements matching link "Home" (Capybara::Ambiguous)
      ./features/step_definitions/navigation_steps.rb:130:in `/^I click "(.*)"$/'
      features/home_page/home_button.feature:28:in `And I click "Home"'
      features/home_page/home_button.feature:23:in `And I click "Home"'

Failing Scenarios:
cucumber features/home_page/home_button.feature:28 # Scenario Outline: Find help with care for elderly, Examples (#2)

does that error message give you an idea about what might be wrong?

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Mar 8, 2017

yeah @tansaku locally rspec is okay but cucumber has two failures. About code climate i think its about the " " and i have changed them to single quotes

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 9, 2017

this is looking great @cesswairimu - you're really nailing these now :-)

So to go to the next level I wonder if we can connect this PR up so that it has a high level description and connects to the PivotalTracker ticket (so that gets auto-closed) when I merge this?

There's a description of the kind of commit message needed here: https://github.com/AgileVentures/LocalSupport/blob/develop/CONTRIBUTING.md#git-comments

And the sort of PR description we would like here:
https://github.com/AgileVentures/LocalSupport/blob/develop/CONTRIBUTING.md#pull-requests

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Mar 9, 2017

@tansaku yes i think that would be brilliant

@cesswairimu cesswairimu force-pushed the cesswairimu:134448159_structured_data_markup_breadcrumb branch from ae74a21 to 4946025 Mar 14, 2017
@cesswairimu cesswairimu force-pushed the cesswairimu:134448159_structured_data_markup_breadcrumb branch from 4946025 to 5e4d749 Mar 15, 2017
* fix Capybara link errors
@cesswairimu cesswairimu force-pushed the cesswairimu:134448159_structured_data_markup_breadcrumb branch from 0e330d3 to d6e35fa Mar 19, 2017
@cesswairimu cesswairimu changed the title WIP 134448159 structured data markup breadcrumb 134448159 structured data markup breadcrumb Mar 19, 2017
@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 20, 2017

@cesswairimu looking good :-)

note that we can make the current page a hyperlink too

BTW, did my suggestions about the PR description and commit messages make sense? Do you need help adjusting the current PR description?

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 20, 2017

regarding the code climate issues we could break the breadcrumb statements out to separate methods from:

  def show
    render template: 'pages/404', status: 404 and return if @volunteer_op.nil?
    @organisation = Organisation.friendly.find(@volunteer_op.organisation_id)
    organisations = Organisation.where(id: @organisation.id)
    @editable = current_user.can_edit?(@organisation) if current_user
    @markers = BuildMarkersWithInfoWindow.with(VolunteerOp.build_by_coordinates, self)
    add_breadcrumb @organisation.name, organisation_path(@organisation)
    add_breadcrumb @volunteer_op.title, :volunteer_op_path
  end

perhaps I can show you how in today's meeting ...?

@@ -130,6 +130,17 @@ def find_record_for(object, schema, name)
click_link(link)
end

When /^I click "(.*)" (.*) link$/ do |link, position|

This comment has been minimized.

Copy link
@sigu

sigu Mar 20, 2017

Member

@tansaku , I had a feeling we could have improved on the previous step regular expression to accommodate this little change introduced

improve this 👇 step

 When /^I click "(.*)"$/ do |link|
   click_link(link)
 end

Looks like having both is not DRY enough

This comment has been minimized.

Copy link
@tansaku

tansaku Mar 21, 2017

Member

maybe we should be using within to isolate the clicking on the breadcrumb via it's id?

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Mar 20, 2017

@tansaku yes i understood the steps in contributing. I will try implement the hyperlinks in the last breadcrumb trail. If i succeed i will make the last commit [Finishes #ticket_id]. Thanks for that

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Mar 29, 2017

@tansaku @sigu The gem i was using for this ticket 'breadcrumbs_on_rails' is set in a way that the last link on the trail is not clickable. Trying to make the last element clickable was unsuccessful.

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Mar 29, 2017

@cesswairimu that's fine - Amazon doesn't make them clickable - let's leave it as it is :-)

…ured_data_markup_breadcrumb
@sigu

This comment has been minimized.

Copy link
Member

sigu commented Apr 24, 2017

@tansaku this looks ready to me for merging. Have a final look and possibly merge it in

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Apr 27, 2017

great stuff @cesswairimu and @sigu - could you make refactoring tickets for each code climate issue so that we have a record of them when I merge?

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Apr 27, 2017

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented Apr 28, 2017

Incorporates markup breadcrumb trail for easy navigation [Finishes #134448159]

@tansaku

This comment has been minimized.

Copy link
Member

tansaku commented Apr 28, 2017

great @cesswairimu - if you could just finally paste in links to refactoring tickets in pivotal tracker corresponding to the code climate issues that aren't being fixed, then we'll be all done :-)

@cesswairimu

This comment has been minimized.

Copy link
Contributor Author

cesswairimu commented May 3, 2017

ticket for refactoring the code climate issues https://www.pivotaltracker.com/n/projects/742821/stories/144806617

@tansaku tansaku merged commit f348936 into AgileVentures:develop May 3, 2017
2 of 3 checks passed
2 of 3 checks passed
codeclimate 3 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 99.455%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.