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

#36 - An Event has many years #66

Merged
merged 1 commit into from Feb 22, 2018

Conversation

Projects
None yet
2 participants
@flyinggrizzly
Copy link
Contributor

flyinggrizzly commented Feb 2, 2018

What does this PR do?

This PR allows an Event to have multiple years instead of each year of an event being a distinct record. It achieves this by splitting the current Event model into Event and EventInstances. The Event model holds the name/brand/identity of the event, and the EventInstance records the year(s) in which it took place.

The Submission model is now related to the EventInstance model.

The changes in the PR are big, affecting:

  • event model becoming two models
  • database migrations
  • events controller becoming two controllers
  • spec changes for the Event, EventInstance, and Submission spec
  • spec changes for the controllers of the above
  • view changes for Events, Event Instances, and Submissions
  • feature spec updates for the above
Why was this work done? Is there a related Issue?

Addresses issue 36, which recommended this feature.

Where should a reviewer start?

Start by testing the database migration's reversibility. It has been tested with the seed data without issue already, but it's my first time writing a migration in two directions by hand, so it's possible I've missed something.

After that, probably best to make sure the spec coverage is sufficient (there isn't testing for the new EventsController because the app has previously only tested the #create action for it, and that has been delegated to the EventInstancesController#create action).

After that, what I've done is compare the live app to this side by side to make sure that behavior changes are sensible (for example, the Speaker creation workflow should not have changed; the event creation workflow should have changed but appear very similar; submission creation workflow should still work the same; there is still no submission editing workflow unless you know a submission ID).

Are there any manual testing steps?


Screenshots

Deployment instructions

Database changes

  • rename events table to event_instances
  • create a new events table
  • populate new events table with names from event_instances
  • remove name column from event_instances table

New ENV variables

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 2, 2018

@nodunayo I can squash this down if you want, but I've left it in case it's easier to parse in its broken down state.

There are a couple of commits with the same name (I was learning how to use vimagit and had a hiccup or two), and one or two that still say ***WIP*** in them, but otherwise they make it pretty clear what changes you're getting (sorry about the goofs though!).

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 2, 2018

I'm having trouble refactoring the issue Code Climate is raising with the filter method--feedback on that would be useful if you've got any!

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 4, 2018

Hey @flyinggrizzly,

Thank you so much for all of your hard work on this. I've seen the other issues you've raised too. This upcoming week is very busy for me (need to do some talk writing and have dance video rehearsals and a shoot) but my aim is to start looking at these as soon as possible!

Thank you very much,

Nadia

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 4, 2018

No worries—good luck with the video and talk and everything!

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 12, 2018

Haven't forgotten about this!! I feel bad that it's been sitting here for over a week...but I hope to start looking soon. I go away for a few days on Thursday but I hope to start looking on Wednesday!

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 12, 2018

Don’t sweat it—I totally know how life gets, though maybe not as much as a super in-demand speaker/music video star 😎

I’ll be a little intermittent come Friday—be in LA for a wedding for a week, then another week in NYC, but I’ll have my laptop and can still write code!

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 13, 2018

Hey!! Finally starting to look at this!

I'm just going to post comments as and when I get to them rather than one huge comment at the end!

For a start — great opening comment. Thank you for the detail, very helpful and exciting!

Re the commits, I don't mind there being more. However, are you open to squashing down some?

For example:

  • Can you squash down the first lot until you get to naming Cohort to EventInstance — given we don't even use cohort, I reckon we can get rid of any record of that change.
  • Can you squash down any commits to do with the 'todo' file into other commits?
  • I'm not a fan of commits that have failing specs. I think you should be able to check out a repo at any point in history and all the test should pass — can we squash together broken test commits with the commits that fix them?

These will allow for a cleaner git history! Hope you're okay with that. Let me know.

}

/**
* 1. Correct `color` not being inherited in IE 8/9/10/11.

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

@flyinggrizzly — are you saying that the change below allows compatibility with IE 8,9, etc..or that it's not working as it is..?

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

This fieldset code is pretty much straight out of Normalize, which I added because I wanted to override what I'm guessing where Skeleton styles where the fieldset legend wasn't inline with the fieldset border or was getting squashed in there (it looked pretty rough).

The comment comes with it, though that link is to what looks to be a newer version of Normalize. My read is that it fixes some weird behaviors with fieldset legends in IE. Skeleton may resolve those in other ways, but for this look I think it's necessary.

That said, CSS is not my strong suit...

This comment has been minimized.

@nodunayo

nodunayo Feb 14, 2018

Owner

Okay, cool!

We'll leave it as is for now.

Thanks for the explanation.


/**
* 1. Correct `color` not being inherited in IE 8/9/10/11.
* 2. Remove padding so people aren't caught out if they zero out fieldsets.

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

I don't understand what you're pointing out here. Can you explain please? Thank you. ☺️

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

See above with thing about comment coming out of Normalize.

@@ -11,4 +11,9 @@ def markdown(text)
markdown = Redcarpet::Markdown.new(renderer, options)
markdown.render(text).html_safe
end

# Returns a list of all extisting event names

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

*existing

elsif event.nil? && new_parent_event_name.blank?
errors.add(:base, 'choose either an existing event or to create a new one')
else
create_event!(name: new_parent_event_name) unless event

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

Where has this method come from? Is it some Rails magic??

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

There's probably room to refactor this method, especially so that the comments aren't needed — and this is where the Code Climate error is stemming from.

Will do some more thinking here and come back to it!

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

ActiveModel#create_association! is added to the model when you call belongs_to, so definitely Rails Magic (TM)!

I'll take a swing at refactoring the branching conditional too. I'm a little hesitant to write more helpers to handle the AND/ORs, but that might be the clearest approach. Will check.

This comment has been minimized.

@nodunayo

nodunayo Feb 14, 2018

Owner

Ooh, you learn something new every day! Cool. :-)

Re the branch conditional..maybe there's room to extract some of the branches into explanatory methods as a first step? I'd need to take a closer look to think about best approach.

<h3><%= link_to @event_instance.event.name, @event_instance.event %> <%= @event_instance.year %></h3>

<h5>Proposals</h5>
<%= render partial: 'shared/instance_submissions', locals: { instance: @event_instance } %>

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

So, I've just learnt that partials have performance implications on Rails sites — for the worse! Interesting, eh? I'm sure it's not a problem for us now, but just interesting to know.

@@ -0,0 +1,66 @@
class RenameEventsToEventInstances < ActiveRecord::Migration[5.1]
# Stub models for migration in case they get renamed in the future

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

I don't understand why you'd need to do this — isn't it that migrations are always run in order in a repo so at this point in time the table names will always be what they are now?

Or do you mean if the table names were to be changed within this commit/PR?

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

It protects the migration from future model name changes. If the migration relies on the model names (which are defined outside of its control), at a hypothetical future date when a model name has changed, the migration will hit Unkown Constant errors when it tries to call methods on a class that no longer exists.

In the migration, the dangerous bit would be this; if Event had been renamed Conference and EventInstance had been renamed EventOccurrence both of these loops would throw:

Event.all.each.do |ev|
  # do stuff with ev
end

#or 

EventInstance.all.each.do |evi|
  # do stuff with evi
end

Basically, the migration can always expect database table names to be events or event_instances at the point that the migration is running, but it has no guarantee that we haven't renamed the EventInstance model back to Cohort or something else.

This comment has been minimized.

@nodunayo

nodunayo Feb 14, 2018

Owner

Right, got it! Of course. We're stubbing models, not the table names! Thanks.

end

def down
revert { remove_event_instance_name_col }

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

Ooh, didn't know you could do this. Interesting!

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

super readable right?

This comment has been minimized.

@nodunayo

Given(/^there is an event called (\w+) that does not have an instance for the year (\d+)$/) do |conf, year|
event = create(:event, name: conf)
if inst = event.instances.find_by(year: year)

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

Hmm...I find this technique, interesting. Never seen it before.

I'm torn by it — as I think when running the test, there shouldn't be anything left in the DB...and I'd want to know if things were being left over...

And yet — if they were, we wouldn't find out via this test..

What about if we changed them to expectations, like: expect(Event.find_by(name: etc..).to eq(false)?

Is that strange because we're running expectations before the end of the test script?

Let me know what you think.

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

So... I had started responding to all of these, and in a previous life of this comment, had talked myself in a loop into thinking this was unnecessary.

TL;DR is that we should trust database_cleaner to do its job, and also assume that Factory Bot won't instantiate a related model apropos of nothing, unless we tell it to.

I think I did this because I wanted the code to reflect the step definition: there is an event, and it must not have an instance for xxxx year. But I think it's too literal now on reflection. Will change.

This comment has been minimized.

@nodunayo

nodunayo Feb 14, 2018

Owner

Okay, cool. :-)

notes.md Outdated
@@ -0,0 +1,4 @@
## Testing steps

This comment has been minimized.

@nodunayo

nodunayo Feb 13, 2018

Owner

Can we get rid of this file? (And if we do squash that into another commit?)

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 13, 2018

Author Contributor

🤦‍♂️

This comment has been minimized.

@nodunayo
@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 13, 2018

Woweee! What a big commit! Well done, though!

So far, I've read through the commits and the code — overall it looks very good and well thought-out. Next step is for me to pull it down and have a play/test the migrations, etc.

In the meantime, if you could address the load of comments I've left, that would be amazing! Let me know if anything I've said/asked isn't clear!

By the way, the test coverage looks great from what I can see. I'll double check the EventsController specs another time.

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 13, 2018

Sorry about the size of the PR... it probably could have been broken down into more and smaller PRs on reflection.

I'm going to squash everything down into a single commit--my plan had been to leave the commit history just for review, and I don't think it will be trivial to squash and pick commits so it's always feature/tests.

Unless you'd rather have multiple commits, in which case I will defintely do it. But if it's not an actual preference then I don't think it's worth it here...

I'll also take a crack at addressing the comments. From them, if you've got another spare second, it would be good to know

  • what you think about the CSS comments (I replied to your comment)
  • what you think about the Given there is an event with no intance for year xxxx feature step definition and explicitly destroying a record that was probably never created. I have now convinced myself this is redundant code, but I did write it for a reason and I'm open to the possibility that I'm now wrong and was right before
@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 14, 2018

Hey @flyinggrizzly,

If you're fine with one single commit, then that is good for me! In the future, for big features, let's work together to think about smaller chunks.

However, I still think this is an amazing piece of work. Thank you so much.

Re your questions:

  • Can we get rid of those CSS comments? We could potentially put a comment that shows the source is Normalise, or we can leave it as is...

  • I think we shouldn't explicitly destroy the records...what do you think about expectations...? Or maybe we just edit the step definitions to obviously imply there shouldn't be an instance, i.e. 'Given there is a new event called, etc..' There is a chance that you're definitely not wrong and I am having a reaction to it because I've not seen it before. I'll have a little dig in old codebases and do some research to work out whether there's a better approach.

@@ -0,0 +1,66 @@
require 'rails_helper'

RSpec.describe EventInstance do

This comment has been minimized.

@flyinggrizzly

flyinggrizzly Feb 16, 2018

Author Contributor

This spec needs to validate the uniqueness of an instance year scoped to the parent event

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 16, 2018

Just a quick one--it definitely feels like an anti-pattern to have an expecattion at that point in the test. Not just because it defies convention, but because it could potentially hide a later actual failure... though that seems like an academic possibility.

More importantly though, I also realized that going forward, the only way to create an event is through the instance, so it would make more sense to say Given there is an event called AwesomeConf with an instance for the year 2014. Later you just add a new instance to the existing event with a different year.

We could still skip the creation of the first instance, but then the feature isn't matching reality as closely.

Events have the capacity for multiple years
- Event Model, which previously had all info about the event occurence,
becomes EventInstance
- a new Event model now contains event identity
- event creation workflow happens thorugh the instance--an event should
not exist unless we have a record of one of its instances
- update specs for changed models and controllers
- update feature specs to reflect new workflows
- update factories
- update related Submission model to communicate with EventInstance
instead of Event

@flyinggrizzly flyinggrizzly force-pushed the flyinggrizzly:events-have-multiple-years branch from a034314 to 01ca04f Feb 16, 2018

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 16, 2018

@nodunayo... I think we're in business! Down to single commit, Code Climate is happy, and I've made all the requested changes.

I'll be in LA in a few hours, but I'll be around off and on--let me know if we need to make any more changes!

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 16, 2018

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 19, 2018

Hey, @flyinggrizzly! Just looked through the code again and all looks good.

I think the spec coverage is there — I guess it makes sense that the EventController specs aren't there anymore. Will check what my spec coverage report says when I pull down the code.

Next order of business will be pulling down the code and playing with the migration!]

Thank you for all of your work on this!

@nodunayo nodunayo merged commit 8daa9a7 into nodunayo:master Feb 22, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 22, 2018

Oh no! Having an issue when trying to deploy on staging:
screen shot 2018-02-22 at 17 27 52

Any ideas?

A quick glance implies that the migration doesn't allow for conferences that may have already been added earlier on in the migration?

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 22, 2018

@flyinggrizzly — Yeah, we're failing on line 15 of the migration file.

ev = MyEvent.create!(name: event_instance.name)

So we need to first check if that event name already exists. If it does, we use that, otherwise we create one. Maybe we need a find_or_create? Do you want to fix this or shall I?

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 22, 2018

@nodunayo that looks like it’s it exactly. I won’t be able to get to this until I’m in the air tomorrow, but I’m happy make the change if you’d rather I do it and don’t mind waiting a bit!

I was thinking just changing the line to find_by :name || create! to avoid extra code in the model stub, but either would totally work!

I’ll probably update the seeds too to make sure that this will get caught in a dev environment (feel a bit silly I haven’t caught this already—the logic was mostly there to handle this expected sitch anyways)

@nodunayo

This comment has been minimized.

Copy link
Owner

nodunayo commented Feb 22, 2018

@flyinggrizzly That's fine — I can wait. If you haven't done it by the weekend, then I'll pick it up, but no pressure/rush!

Yes — your suggestion looks good.

I feel silly that I didn't catch this when reading your code! Oh well! We can both feel a little bit silly together and now stop feeling silly because stuff like this always happens! 😁

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 23, 2018

@nodunayo--this should be ready to go.

Steps for verifying migration:

before pulling down latest changes (with --force)

  1. seed the DB on the feature branch (it creates events with multiple instances which we can use to replicate the situation on staging)
  2. roll back the last migration: bin/rails db:rollback STEP=1--this sets it so we have multiple old events with the same name but different years
  3. try to migrate forward again, get failure

after pulling down latest changes

repeat as above, but the migration will succeed because it will try to find an existing event before attempting to instantiate it

@flyinggrizzly

This comment has been minimized.

Copy link
Contributor Author

flyinggrizzly commented Feb 23, 2018

New PR opened #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.