Project Page and Low Hanging Fruit #64

sumnerevans · 2018-03-17T07:15:52Z

Included in this PR is my work on the project page as well as work done by Liam and David on the low hanging fruit issues.

Resolves /s issues #56
Resolves /u without a username fails with error 500 #55
Resolves Add a project listing page #13 (See Project Listing Page #35 for relevant screenshots)

Merging in survey bugs fixes

Project Listing Page

samsartor · 2018-03-17T16:08:11Z

acmwebsite/controllers/root.py

@@ -47,10 +48,11 @@ class RootController(BaseController):
    mailinglist = MailingListController()
    u = UsersController()
    m = MeetingsController()
-    s = SurveysController()
+    survey = SurveysController()


There is code elsewhere that relies on the /s endpoint.

samsartor · 2018-03-17T16:11:22Z

acmwebsite/controllers/root.py

    schedule = ScheduleController()
    error = ErrorController()
    contact = ContactController()
+    projects = ProjectsController()


This controller needs to be renamed. It is getting too confusing. So far, a ThingsController has been responsible for looking up a thing, not for listing all things. This controller should really be called ProjectListController.

samsartor

Revert or fix #61.

samsartor · 2018-03-17T16:21:41Z

#61 needs to be fixed. At the very least it breaks /attend. Apparently we need to do more testing as part of the review process.

sumnerevans · 2018-03-17T16:28:25Z

@samsartor, no, /attend needs to be changed. See #56 (comment) for details on why we are changing this.

… with it

Reverted changes. Deferring to #65.

jackrosenthal · 2018-03-20T17:51:40Z

acmwebsite/lib/helpers.py

@@ -26,7 +26,11 @@ def markdown(*args, strip_par=False, **kwargs):


 def icon(icon_name):
-    return Markup('<i class="glyphicon glyphicon-%s"></i>' % icon_name)
+    return Markup('<i class="glyphicon glyphicon-{}"></i>'.format(icon_name))


Glyphicons are gone in Bootstrap 4... is this ever called anyway?

We use it in acmwebsite/templates/survey_results.xhtml to display the up and down arrows on the survey results page. We can easily replace those though. Added a note on #67 about this.

jackrosenthal · 2018-03-20T17:53:50Z

acmwebsite/public/js/mailinglist.js

-    });
+  $('#mailinglist-form').submit(function() {
+    $(this).find("button[type='submit']").prop('disabled',true);
+  });


Why did we make changes to this file?

jackrosenthal · 2018-03-20T17:54:16Z

acmwebsite/public/js/site.js

+  $('input[name="first_time"]').change(function() {
+    checked = this.checked;
+    $('.on-first-time').each(function() {
+      if (checked) { $(this).fadeIn() } else { $(this).fadeOut() }


Same here... not sure how this is related to your PR.

Let's stick to 4 spaces for indentation on all code, two spaces on markup. Seems more consistent than this silly 2-space JavaScript thing.

I've updated the code. However, you can't just go around changing the contributing guidelines on a whim like this again. It is no longer just you, Sam, and me on the project.

In the future, please inform all contributors (by email or some other method) before changing the contributing guidelines so that we can all be on the same page.

jackrosenthal · 2018-03-20T17:55:10Z

migration/versions/d0578a57e0f9_projects.py

+
+def downgrade():
+    op.drop_table('team')
+    op.drop_table('projects')


Make sure the migrations work... I have not tested as a part of my review.

Migration tested. It does work.

jackrosenthal · 2018-03-20T17:56:50Z

acmwebsite/controllers/project.py

+
+    @expose('acmwebsite.templates.projects')
+    def index(self):
+        return dict(page='projects', projects=DBSession.query(Project).all())


No need to .all() here I think... you are just iterating over it once it looks like.

jackrosenthal · 2018-03-20T17:57:01Z

acmwebsite/controllers/project.py

+
+
+class ProjectsController(BaseController):
+    """Root controller for listing all projects"""


"Root controller"?

jackrosenthal · 2018-03-20T17:57:47Z

acmwebsite/model/__init__.py

@@ -62,6 +62,20 @@ def init_model(engine):
 from acmwebsite.model.meeting import Meeting
 from acmwebsite.model.mailmessage import MailMessage
 from acmwebsite.model.survey import Survey, SurveyField, SurveyResponse, SurveyData
+from acmwebsite.model.project import Project, team_table


Why is team_table exposed?

out of date

jackrosenthal · 2018-04-08T21:37:44Z

Yes, this is fine... in the future please try and keep pull request topics separate... for example, changing style of code in separate request.

sumnerevans and others added 15 commits December 15, 2017 23:41

#13 project database model, controller, preliminary UI

b10a2df

#13 added pretty project display

7d18779

#13 Clean up a couple of minor things

2e7a7cf

Add video URL field

0dc5ebd

#13 remove stuff regarding submitting projects

61c50bf

#13 cleaning up project page

571afbf

#13 fixed indentation, removed printf formatting

5f1aefd

#13 put project listings into panels

f7b0e68

#13 improved project listing styles

23071bf

404 on no username

912f624

Got rid of the "and User != "

6c55054

Fixxed Survey bug and changed /s to /survey

81116ef

Merge pull request #61 from ColoradoSchoolOfMines/surver-Bugs

700dfb6

Merging in survey bugs fixes

Merge develop into project-page

4421d7a

Merge pull request #35 from ColoradoSchoolOfMines/project-page

5d8bab2

Project Listing Page

sumnerevans assigned samsartor and sumnerevans Mar 17, 2018

sumnerevans requested a review from jackrosenthal March 17, 2018 07:15

samsartor reviewed Mar 17, 2018

View reviewed changes

samsartor previously requested changes Mar 17, 2018

View reviewed changes

sumnerevans added 5 commits March 17, 2018 13:14

Revert part of 81116ef: back to /s for surveys for now, #65 will deal…

0cac685

… with it

Fix migrations

f8c8d02

Merge branch 'project-page' into develop

97931ef

Fix bug with project links

2a6807e

Fix issue in User controller

7eb29ee

jackrosenthal previously requested changes Mar 20, 2018

View reviewed changes

Warfields mentioned this pull request Mar 20, 2018

Presentation Slide Downloads #68

Closed

#64 updates from code review

c8baf45

sumnerevans merged commit bb5f59b into master Apr 8, 2018

samsartor deleted the develop branch April 11, 2018 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project Page and Low Hanging Fruit #64

Project Page and Low Hanging Fruit #64

sumnerevans commented Mar 17, 2018

samsartor Mar 17, 2018 •

edited

Loading

samsartor Mar 17, 2018

samsartor left a comment

samsartor commented Mar 17, 2018 •

edited

Loading

sumnerevans commented Mar 17, 2018

jackrosenthal Mar 20, 2018

sumnerevans Mar 21, 2018

jackrosenthal Mar 20, 2018

jackrosenthal Mar 20, 2018

jackrosenthal Mar 20, 2018

sumnerevans Mar 21, 2018

jackrosenthal Mar 20, 2018

sumnerevans Mar 21, 2018

jackrosenthal Mar 20, 2018

jackrosenthal Mar 20, 2018

jackrosenthal Mar 20, 2018

jackrosenthal commented Apr 8, 2018



		class ProjectsController(BaseController):
		"""Root controller for listing all projects"""

Project Page and Low Hanging Fruit #64

Project Page and Low Hanging Fruit #64

Conversation

sumnerevans commented Mar 17, 2018

samsartor Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsartor left a comment

Choose a reason for hiding this comment

samsartor commented Mar 17, 2018 • edited Loading

sumnerevans commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackrosenthal commented Apr 8, 2018

samsartor Mar 17, 2018 •

edited

Loading

samsartor commented Mar 17, 2018 •

edited

Loading