Rename 'ConferenceController' to 'ConferencesController' and 'ProposalController' to 'ProposalsController'

[richiethomas]

Issue #1120

[richiethomas]

By the way, I noticed that the following 4 views contained code which references the singular "/conference/" resource (as opposed to the plural "/conferences/"), but these views did not break any tests:

app/views/admin/events/registrations.html.haml
app/views/admin/events/index.html.haml
app/views/admin/events/_proposal.html.haml
app/views/admin/roles/_users.html.haml

This implies to me that they are not under test coverage. Would you like me to add that coverage and make sure they render? If so, please let me know what other happy/alternate/sad paths I should cover besides making sure they simply render properly. Cheers!

[richiethomas]

Hi, I checked out the stacktrace and this build failure appears to be because of a failure to install the PhantomJS gem in the test environment. See line 674 of the stack trace:

bunzip2: phantomjs-2.1.1-linux-x86_64.tar.bz2 is not a bzip2 file.

Since I didn't make any changes to the Gemfile, would the best option be to re-trigger the build and see if the error goes away?

[richiethomas]

Getting the PhantomJS download error again. I Googled the error, looks like it might have something to do with a rate-limiting issue when downloading gems in a CI environment:

colszowka/phantomjs-gem#70

[differentreality]

What's that? :)

[richiethomas]

Oh my... I'll remove that and push again. :-) haha that was me trying to learn where the partial started and ended. Just random text that I knew would be unique within the DOM.

[differentreality]

@richiethomas very nice work! Thank you for the contribution :-)

Why don't you also mention the issue number on the description so that issue and PR have a reference of each other?

The error in travis is not caused by your PR, I have restarted it now.

The last commit is not relevant to this PR though. Do you think you can remove it from this PR and open a new PR about it?

[richiethomas]

@differentreality re: removing the last commit- sure thing, I guess the easiest thing would be to push a reverted commit to this PR, wait til this issue is closed, then revert the revert and push again. Does that sound right?

[differentreality]

If you mention #1120 in any comment (in my previous comment I meant the first comment on this PR) then both the issue and this PR will have a reference to each other. That's very helpful.

For this PR, things look good to me!
I would rather you squashed the commits actually. These are way too many commits with very little change each. If you need help with that, we can do it together tomorrow or on Tuesday.

For the commercials issue, you can do:

git checkout master
git checkout -b commercials_indication
git cherry-pick b4d643a9b0becd67ff19d71cde6a982544225eeb 

(this number is the hash of your commit)

That will create the same commit in your new branch. You can check the git log to verify, and then git push the new branch.
If things don't make total sense, feel free to leave a comment here, or find me on IRC, or we can schedule a specific time to go over things in more detail, if you want to.

[richiethomas]

OK, just saw the description area and added #1120 to it. I can squash the commits, but if I push again to my fork, will that mess up the commit history? Or will it just replace the multiple small commits with the one large one?

Going forward, do you prefer I simply push one large commit, or was it helpful to first review the smaller commits individually?

[richiethomas]

For question 1 above, I think I answered my own question. I squashed the commits and pushed again, so we'll see what happens. :-)

[richiethomas]

Hey there, looks like this breakage is due to an expectation problem:

Expectation: Towne-O'Reilly Office
Actual: Towne-O'Reilly Office

This is probably why it passed repeatedly on my local- the name is dynamically generated by Faker and it's likely that none of the generated names had apostrophes. Let me try to rewrite the expectation so it can handle special characters.

[differentreality]

The failed test was a false positive actually, I have restarted travis, and eventually it should be fine.

[differentreality]

BTW you have not squashed your commits, there are still 27 commits here.

It is always good if your commits represent a group of changes that make sense on their own. Once you open a PR, I prefer that you only push new commits afterwards, because it helps with review (since I can see the new commit and see only new changes made). However sometimes during review we accumulate lots and lots of new commits with small changes, so then, when the PR is ready to be merged, it makes sense to squash commits into 1 or just a couple meaningful commits.
Does all that make sense? As you contribute more - which I hope you will - and open new PRs, we will see all those things in practice.

[richiethomas]

I squashed the commits on a local branch, but forgot to rebase to master before pushing from master. Now that my fork has all 27 commits, I am getting the error "Updates were rejected because the tip of your current branch is behind" etc etc. Would the best plan be for me to force push the squashed commit to my fork? I know the dangers of force pushing, so a 2nd opinion from you would be welcome, before I proceed. :-)

[richiethomas]

This is strange- now that I've squashed the commits, I'm seeing 2 new failures on my local. They are the following:

1. Admin::ConferencesController organizer access behaves like access as organizer GET #index with more than 0 conferences populates an array with conferences
   Failure/Error: expect(assigns(:conferences)).to match_array([conference, con2])

2. Api::V1::RoomsController GET #index without conference scope returns all rooms
   Failure/Error: expect(json.length).to eq(2)

   expected: 2
        got: 9

For the first, it's expecting an array of 2 conferences, but it's getting an array of 35 total. After adding a 'byebug' as the first line of the test, and checking for active and inactive conferences, it looks like there are 34 conferences already generated somewhere. I haven't investigated the 2nd error yet but I'm confused why I didn't see either of these failures before, either locally or on Travis. I observed these failures when rolling back to d73bb61, both before and after squashing.

I need to step away now but will continue investigating later. In the meantime, any advice would be appreciated.

[richiethomas]

Huh, the failures appear to have gone away on their own. That's great and disconcerting at the same time. :-) Anyway, the squashed commit is mostly the same, the only difference was that I caught some additional references to the "/conference/" path pattern, and changed them to "/conferences/" instead. The files were:

1. /app/views/admin/events/_proposal.html.haml:

old: method: :patch, url: "/admin/conference/#{@conference.short_title}/program/events/#{@event.id}?event[is_highlight]=",
new: method: :patch, url: "/admin/conferences/#{@conference.short_title}/program/events/#{@event.id}?event[is_highlight]=",

old: method: :patch, url: "/admin/conference/#{@conference.short_title}/program/events/#{@event.id}?event[require_registration]=",
new: method: :patch, url: "/admin/conferences/#{@conference.short_title}/program/events/#{@event.id}?event[require_registration]=",

2. /app/views/admin/events/index.html.haml:

old: method: :patch, url: "/admin/conference/#{@conference.short_title}/program/events/#{event.id}?event[require_registration]=",
new: method: :patch, url: "/admin/conferences/#{@conference.short_title}/program/events/#{event.id}?event[require_registration]=",

old: method: :patch, url: "/admin/conference/#{@conference.short_title}/program/events/#{event.id}?event[is_highlight]=",
new: method: :patch, url: "/admin/conferences/#{@conference.short_title}/program/events/#{event.id}?event[is_highlight]=",

[differentreality]

Please fix this title to reflect what we test for (ie. redirection to conferences_path, not root_path)

[differentreality]

What about:

https://github.com/openSUSE/osem/blob/master/app/views/admin/venues/show.html.haml#L11
https://github.com/openSUSE/osem/blob/master/app/views/conference/_venue.html.haml#L5
https://github.com/openSUSE/osem/blob/master/app/controllers/admin/contacts_controller.rb#L6

[richiethomas]

Nice catch. I'll push fixes for the 2 view files.

The references to "conference/" in the ContactsController all look to be referring to a specific conference id, since they relate to the '#show', '#edit', and '#update' actions. Also, they are comments, so they won't break any tests (though I would be inclined to pluralize the comments to ":conferences/" if they referred to multiple conferences). If you'd like, I could change the comments from ":conference/" to ":conference_id/", to more closely match what gets output from (for example) 'rake routes'.

What do you think?

[differentreality]

Nah, you are right, you don't need to change the comments!

[richiethomas]

2 view files are fixed, as is the incorrect spec title in application_controller_spec. Ready for review. :-)

[differentreality]

@richiethomas we need to rebase this. But I just noticed you don't have a branch with your changes? Please find me on IRC to see how we should proceed.

[richiethomas]

@differentreality I just joined the Freenode room. Trying to @mention you but I've never used IRC before so not sure if I'm doing it right. :-)

[differentreality]

@richiethomas you did it fine, but you need to stay online on IRC for me to talk to you.

I am noticing in this PR that you have not used a new branch to submit this PR.
You need to have your changes in a branch and your master needs to be the same as the remote master.
Try the following from your master:

git checkout -b pluralize
git log

(Check if you see your commit on the top, if not, do not proceed!)

git checkout master
git pull origin master
git checkout pluralize
git rebase origin master
git push

open a PR with the new branch.