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

Add partials views and client views #103

Merged
merged 11 commits into from
Aug 24, 2016
Merged

Add partials views and client views #103

merged 11 commits into from
Aug 24, 2016

Conversation

alxngyn
Copy link
Contributor

@alxngyn alxngyn commented Jun 27, 2016

PR 1 of 3: clients
Related issue: #102
This is a prototype view of the clients endpoint.

Changes in this PR.

  • Client View for CREATE, EDIT, INDEX (list view), SHOW(detailed view)
  • Partial views for headers and footers in /views
  • No delete view implemented as I was running into ForeignConstraint Restrictions and still need to figure out the model issue(if we need should even be deleting things)
  • delete now soft deletes and hides the client from the list view

Testing this PR.

  1. sudo docker-compose run --service-ports dev bash
  2. run rake migrate to setup the DB schema
  3. Generate data with this code: (copy and paste it into a temp rb file and run it)
require './environment.rb'
require 'models'

# generate data
 (0...25).each do
   o = [('a'..'z'), ('A'..'Z')].map(&:to_a).flatten
   name = (0...10).map { o[rand(o.length)] }.join

   new_client = Client.create(name:  name,
                              contact_name:  'bob',
                              contact_email:  'bob@example.com',
                              description:  'blah')

   name = (0...10).map { o[rand(o.length)] }.join

   new_project = Project.create(name: name,
                                client_id: 1 + rand(new_client.id),
                                resources: 'node,thingy',
                                description: 'blah')

   name = (0...10).map { o[rand(o.length)] }.join

   NodeResource.create(project_id: 1 + rand(new_project.id),
                       name: name,
                       type: 'ganeti',
                       cluster: 'cluster1.osuosl.org',
                       created: DateTime.now,
                       modified: DateTime.now)
 end

  1. In a web browser, go to http://localhost:4567/clients
  2. Try clicking on the client name, the edit button, using the edit form, etc

Expected Output.

n/a

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 27, 2016

blocked by #87
The patch and delete methods needs method_override to be enabled to function properly
EDIT: #87 merged.

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 28, 2016

Screenshots of the views:
Edit:
clients_edit
Show:
clients_show
Create:
clients_create
Index:
clients_index

@pop
Copy link
Contributor

pop commented Jun 28, 2016

@alxngyn How did you populate the DB with dummy data? The script keeps breaking (as you point out it will).

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 28, 2016

Did the code not work for you? (I mentioned that the script keeps breaking?)
It should work if you put it into a file on the same directory level as app.rb.

@pop
Copy link
Contributor

pop commented Jun 28, 2016

@alxngyn This looks like the error you were describing in the last checkbox for this PR. I mis-interpreted what you meant by that, but the point still stands.

[root@aa55c89eb34a code]# ruby tmp.rb                                                                                                                         
/usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:2181:in `block in set_restricted': method name= doesn't exist (Sequel::MassAssignmentRestriction)
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:2168:in `each'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:2168:in `set_restricted'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:1597:in `set'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:2124:in `initialize_set'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:1199:in `initialize'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:170:in `new'
        from /usr/local/lib/ruby/gems/2.3.0/gems/sequel-4.33.0/lib/sequel/model/base.rb:170:in `create'
        from tmp.rb:9:in `block in <main>'
        from tmp.rb:5:in `each'
        from tmp.rb:5:in `<main>'

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 28, 2016

@ElijahCaine Oops my fault, you need to run rake migrate before running that script to get the DB schema setup. I'll add it to the PR testing steps.

<th>Client Name</th>
<th>Contact Name</th>
<th>Contact Email</th>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with the spacing here?

@pop
Copy link
Contributor

pop commented Jun 28, 2016

@alxngyn Feedback:

  • Can we add an 'edit' button to each client page? I don't like that I have to go to /clients/ to edit a client.
  • Can we test these views? We don't necessarily need to, but it'd be nice to have something automatic that checks what we're creating.

All-in-all this PR is awesome. Bootstrap is a good choice, it's very easy to navigate. Once we finish all of the endpoints there might be some polish to make it even more user-friendly, but right now I'm just impressed.

+1

@alxngyn
Copy link
Contributor Author

alxngyn commented Jun 28, 2016

@ElijahCaine
I believe I can make a tests to make sure each route implements the correct .erb file but I don't think it gets any deeper than that. I'll look into it and implement what I find.

@pop
Copy link
Contributor

pop commented Jun 28, 2016

@alxngyn Awesome. The OSL doesn't really do any automated browser testing, and it isn't entirely necessary for this project, but that may also be something to explore.

But like... don't worry about it too much. Any testing is just better than no testing.

@pop pop added Needs Work and removed Blocked labels Jul 1, 2016
@pop
Copy link
Contributor

pop commented Jul 1, 2016

Added needs work label b/c there's no delete tests / we need to figure out how to deal with deleting stuff. (As per the un-checked checkbox in the PR)

@alxngyn
Copy link
Contributor Author

alxngyn commented Jul 6, 2016

@ElijahCaine
Should we be deleting stuff?I feel as a metrics webapp the more data the better. Even if they are no longer a client or project we should still store the data.

@Kennric
Copy link
Contributor

Kennric commented Jul 7, 2016

I think rather than having a delete functionality, we should have an active flag and be able to turn clients (and projects) 'off' for purposes of viewing them in the interface

@alxngyn
Copy link
Contributor Author

alxngyn commented Jul 7, 2016

@Kennric
I agree.
Should i change all the models so they have an active flag? (clients, projects, plugins, node_resources)

@alxngyn alxngyn self-assigned this Aug 4, 2016
@hsolorza hsolorza added this to the Sprint for 08/18 milestone Aug 11, 2016
@alxngyn
Copy link
Contributor Author

alxngyn commented Aug 18, 2016

now that #131 is merged into develop. I can rework the views to 'hide' the data if the active flag is turned off.

</tr>
</thead>
<tbody>
<% for @project in @projects %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace problems here too

@alxngyn
Copy link
Contributor Author

alxngyn commented Aug 19, 2016

I reworked the views/routing to incorporate the active flags now

<thead>
<tr>
<th>id</th>
<th>project Name</th>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Project should be capitalized

@hsolorza
Copy link
Contributor

hsolorza commented Aug 22, 2016

@alxngyn What are the 'link link link..' in the footer going to be linking to? Should they have a name?

@alxngyn
Copy link
Contributor Author

alxngyn commented Aug 22, 2016

@hsolorza those are just place holders for now. I don't think we had any feedback or requirements on the header/footer stuff yet.

@hsolorza
Copy link
Contributor

Everything looks good +1

@Kennric
Copy link
Contributor

Kennric commented Aug 22, 2016

Are there still whitespace issues in views?

@alxngyn
Copy link
Contributor Author

alxngyn commented Aug 22, 2016

@Kennric Only took 4 commits but i fixed the white space and it should show up fixed on Github

@pop
Copy link
Contributor

pop commented Aug 23, 2016

This is a good start, but we'll need to do some serious cleanup with @subnomo to get this to be usable. It's still pretty rough around the edges.
+1 we can merge this.

@alxngyn alxngyn merged commit e5560da into develop Aug 24, 2016
@Kennric Kennric deleted the alxngyn/client-view branch June 20, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants