Presidents Timeline in Committee Members Page - #490
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “Presidents Timeline” visualization to the Committee Members page, sourcing presidential term data from existing committee YAML data and computing timeline segments in CommitteeHelper.
Changes:
- Extend committee data YAML to include president term start/end dates (including missing historical president entry data).
- Render a timeline chart on
committee-memberspage using helper-provided president/term calculations. - Add a helper spec to validate the presidents data pipeline.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
app/helpers/committee_helper.rb |
Adds logic to load, group, and compute timeline term segments for presidents. |
app/views/pages/committee-members.html.erb |
Renders the presidents timeline UI above the committee members grid. |
config/data/committee.yml |
Adds start/end dates for the current president’s term. |
config/data/previous.yml |
Adds/updates historical president term ranges and appends a missing president entry. |
spec/helpers/committee_helper_spec.rb |
Introduces initial tests for CommitteeHelper#presidents. |
| def load_presidents | ||
| current_committee = YAML.load_file Rails.root.join('config/data/committee.yml') | ||
| previous_committee = YAML.load_file Rails.root.join('config/data/previous.yml') | ||
|
|
||
| (current_committee + previous_committee) | ||
| .filter { |member| president?(member) } | ||
| .map { |p| { name: p['name'], start: to_array(p['start']), end: to_array(p['end']) } } | ||
| end |
There was a problem hiding this comment.
I'm not sure the consequence, but CommitteeHelper is included in ApplicationHelper already. I suppose I can keep this as is since priority by Copilot seems to be low. @simonhildebrandt , what do you think?
There was a problem hiding this comment.
I'm agonising over this a bit, because while I generally think Copilot is right about removing the duplication, I'm a bit against the Rails magic of code in Helper files, because I'm suspicious of their ability to call each other. So on balance I think you're OK to keep them where they are, for the moment.
I think the fix for the future would be to load the YAML files once and cache them - they could be part of the Rails configuration, or stashed on the current thread - anything to save us loading them for every request (and in the case of def committee, every usage).
There was a problem hiding this comment.
Thanks for your review on this too. I will leave this as is for now. The future fix idea sounds ideal 🙂
| describe '#presidents' do | ||
| it 'returns a list of presidents' do | ||
| expect(helper.presidents).not_to be_empty | ||
| end | ||
|
|
||
| it 'has Keith as the oldest president' do | ||
| expect(helper.presidents[-1][:name]).to eq('Keith Pitty') | ||
| end | ||
|
|
||
| it 'is 2012/06/16 when the second oldest president term starts' do | ||
| expect(helper.presidents[-2][:start][0]).to eq('2012-06-16') | ||
| end | ||
| end |
There was a problem hiding this comment.
One way to address this might be to mock the YAML loading part of this, so you can control the data - something like:
before do
allow(YAML).to receive(:load_file).with(Rails.root.join('config/data/committee.yml')).and_return([{name: 'Nick Wolf', etc}])
end
There was a problem hiding this comment.
Thanks @simonhildebrandt for your advice! I added mock and adjusted tests. I also bunched those tests into one.
|
@simonhildebrandt , I have made adjustments according to your advice and copilot suggestions. Please see comments for each item and review those updates. |
| @@ -1,12 +1,17 @@ | |||
| <% title "Previous Committee Members" %> | |||
| -<% title "Previous Committee Members" %> | |||
There was a problem hiding this comment.
Was this dash intentional? Or maybe a merge issue, or typo? (I just ask 'cause it looks slightly strange. 😅 )
There was a problem hiding this comment.
Thanks for noticing this little strange dash. I removed it now.
simonhildebrandt
left a comment
simonhildebrandt
left a comment
There was a problem hiding this comment.
The mocks look great, and so does the result. Well done!
(One small question on the committee members template, but I won't hold up the PR for that.)
I added a timeline for Ruby Australia presidential history to the current committee members page based on the idea by @ZimbiX . Please refer below and review this request.
I used the existing yml files (committee.yml for the current president and previous.yml for all of previous presidents) to get president names, term start and term end. Presidents data is sourced from forum archive, Slack announcements and commits to those yml files. All logic for this timeline lives in the committee_helper file.