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

Knowledge base page #50

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Knowledge base page #50

wants to merge 5 commits into from

Conversation

stevelaskaridis
Copy link
Contributor

Created a 3rd page in the webpage where the presentations available from past meetups are presented to the user.

More specifically:

  • Added the page, creating a new view (app/views/knowledge/index.html.erb)
  • Changed the layout view to provide access to the new view
  • Minor addition in custom.css
  • Created a controller for the new view (app/controllers/knowledge_controller.rb)
    • support for sorting
  • Added the relevant route
  • Created model Presentation class (app/models/presentation.rb)
  • db:migration and db:seed functionality with all the presentations of meetup I could find (not all of them are available)
  • Documented extra step for db creation, migration and seed inside readme.

Of course, if additional presentations are available, they could be included in the database and rendered in the relevant view.

Screenshot preview:

knowledge-base-page

@petros
Copy link
Member

petros commented Jan 5, 2015

Nice work @stevelaskaridis! I like your enthusiasm 😄

A couple of questions:

  • Why is the controller called knowledge and the model presentation?
  • Not sure if we are ready to start a database yet, but I need @xarisd's opinion on that.

@xarisd
Copy link
Member

xarisd commented Jan 5, 2015

Hey guys! Happy new Year!

a couple of points :

  • Controller should be named PresentationsController according to the model for better/easier understanding. Views should be moved accordingly, routes should be changed also.
  • I think that we should avoid ActiveRecord for the time being. WE WILL introduce it at a later stage. We are trying to get there step by step (i.e. see the 'MembersController', no ActiveRecord code there for the 'Member' model).
  • Attribute should be presenter instead of presentor (https://translate.google.com/#en/el/presenter).

@stevelaskaridis you can

  • Keep the model Presentation but do not subclass from ActiveRecord::Base. Instead create the attributes with attr_accessor.
  • Create an initialize method that takes a Hash as an argument and initializes the objects attributes. something like:
def initialize(attrs)
  @title = attrs[:title]
  @url = attrs[:url]
  # ...
end
  • Create a class method named all in your model that will return all of the records. In its implementation just copy the code in db/seeds.rb and at the end return an Array of Presentation objects. Something like the following:
def self.all 
  presentations = [
    {:title => 'git and GitHub', :url => 'https://github.com/thessrb/meetup/blob/master/meetups/2/presentations/gitAndGithub/AppCampGr2011Preso.pdf?raw=true', :date_presented => '06/09/2011', :presentor => 'Petros Amiridis'},
    {:title => 'Simple Web Apps with Sinatra', :url => 'http://prezi.com/de5yqj9sa9zq/simple-webapps-with-sinatra/', :date_presented => '06/09/2011', :presentor => 'Vassilis Rizopoulos'},
   # ...
  ]

  presentations.map do |presentation|
    Presentation.new(presentation)
  end
end
  • Remove (keep it in a branch maybe...) all the Database related files.
  • This way you won't need any changes to the controller and views and we will be able to refactor it when ActiveRecord will be introduced.

Bonus step: Open a new Issue or separate Pull Request to do the same with the MembersController and Member (model class does not exist yet) so we can separate the 'data access' code in the model.

Oh! @stevelaskaridis thank you very much for the contributions AND the very active participation!

@stevelaskaridis
Copy link
Contributor Author

Happy new year from me as well!

@petros, answering to your 1st question, the model (presentation) is more specific than the knowledge page and its equivalent controller. This page could potentially include non-presentation stuff, such as links to code repos or infographics, which do not constitute presentations in the strict sense, nor do they would they share the same attributes. As a result, I opted for this distinction between model and view/controller.

@xarisd, I believe that the aforementioned argument addresses your 1st point as well.
Of course, you are right about "presenter" vs. "presentor". Thanks!

Moreover, I understand if you don't want to involve the database yet. I guess I could make some minor changes to "fix" that.
Alternatively, I could prepare some presentation introducing ActiveRecord and additionally go through this pull request step-by-step if it's in the roadmap of the next meetup and it's ok with you.

@petros
Copy link
Member

petros commented Jan 6, 2015

the model (presentation) is more specific than the knowledge page and its equivalent controller. This page could potentially include non-presentation stuff, such as links to code repos or infographics, which do not constitute presentations in the strict sense, nor do they would they share the same attributes. As a result, I opted for this distinction between model and view/controller.

That's an interesting idea, and I am glad you are trying to design a system that can be easily extended!

However, I still encourage you not to do that 😄. I know, it's contradictory, but when I design something, I prefer not to get ahead of myself. I prefer going through all the steps of a process without jumping ahead—skipping steps.

I generally embrace refactoring, and I prefer spending time to make a system easy to refactor—by adding tests for example—, rather than trying to think of all the possible ways a system might be extended in the future, and try to implement that in one go.

Of course, it's a game of balance. It doesn't have to be simpler than how simple it should be, but still I prefer all of us to err on the side of simple.

Having said that, I suggest you stick to PresentationController, and Presentation model. If we need to add more similar things in the future, and have a page that lists the different entities, we may choose different ways to implement that. An example might be to have a Knowledge model, that's an interface, and then have various models, like Presentation, that among other things, have to implement various methods/attributes.

What we want to avoid generally is the code smell of having a lot of if-elsif statements, instead of a cleaner & simpler code.

But we don't have to worry about that, until the time comes to worry about it ;).

Alternatively, I could prepare some presentation introducing ActiveRecord and additionally go through this pull request step-by-step if it's in the roadmap of the next meetup and it's ok with you.

That's a great idea, especially if we are ready to start covering ActiveRecord. Maybe @xarisd can comment here.

I still prefer first doing it without a database, and then adding the database, because it would be really useful as a teaching tool for people that will check the history of changes. It will also help you if you want to present ActiveRecord, because you will be able to show the transition between not using a database, and using one.

@xarisd
Copy link
Member

xarisd commented Jan 7, 2015

Hi there!

We should continue WITHOUT ActiveRecord for the time being. I totally agree with @petros.

@stevelaskaridis you can prepare a presentation on ActiveRecord at a later stage (a couple of meetups later).

We have two or three things to cover until then, all of which are related to Views and Controllers.

I want to mention that @gmanolis also offered to cover this topic, but since it is a big one we can schedule more than one presentations/sessions for it.

And a comment on "Knowledge Base vs Presentations" issue: For the time being we do not have anything else to show on a "Knowldge Base" page so we should stick to a "Presentations" page.
Later on we can change it (and DEMONSTRATE the change, i.e. how to use a single Controller/Action/View with multiple Models/Partials/Presenters to do this).

So to recap...

  1. Rename the controller/views/route
  2. Change the code NOT TO use ActiveRecord.

We will go there...I promise, but let's not forget that this is an educational project.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants