Projects: string to symbol for fetching published projects

[Erin007]

As part of the ongoing investigation into an issue where only a limited number of Dance Party projects are displaying in the public gallery, I found and fixed a potentially related bug.

Calls to api/v1/projects/gallery/public/all/100 were returning projects of all types as expected; however calls to the api with specific project groups for example, api/v1/projects/gallery/public/events/100 or api/v1/projects/gallery/public/dance/100, were unexpectedly returning empty arrays even when there were projects of types in the specified group in the database.

BEFORE:

The problem occurred because the api route with a specific project group param, calls fetch_published_projects, which in turn calls fetch_published_project_types, which expects the project group to be a symbol, not a string. The project group needs to be a symbol because it is used to key into the PUBLISHED_PROJECT_TYPE_GROUPS hash.

AFTER:

[islemaster]

🎉 Good find!

Regression test? This sort of issue suggests we're either missing tests, or our tests don't reflect how we're really using the API.

[Erin007]

@islemaster In this case, it looks like since we stub the response from the Pegasus database,

code-dot-org/dashboard/test/controllers/api/v1/projects/public_gallery_controller_test.rb

Line 24 in ecf7fcd

PEGASUS_DB.stubs(:[]).returns(db_result(stub_projects)).then.returns(db_result([]))

, our tests don't actually call fetch_published_project_types so they never have to key into PUBLISHED_PROJECT_TYPE_GROUPS. I'm not sure how to add a regression test for this specifically.

[davidsbailey]

@islemaster I agree that adding a regression test is a good idea in principle but it seems like a poor bang-for-the-buck in this situation. Let me explain:

We already have this regression test, validating that a "bogus" project type can't be passed in:

code-dot-org/dashboard/test/controllers/api/v1/projects/public_gallery_controller_test.rb

Lines 63 to 67 in ecf7fcd

	test_user_gets_response_for(
	:index,
	name: 'bad request when project type is invalid',
	params: -> {{project_type: 'bogus', limit: 1}},
	response: :bad_request,

We also have this test, which is testing the right input, but doesn't catch it because we stub out the DB response:

code-dot-org/dashboard/test/controllers/api/v1/projects/public_gallery_controller_test.rb

Lines 91 to 93 in ecf7fcd

	test 'project details are correct listing published applab projects' do
	Rails.stubs(:env).returns(ActiveSupport::StringInquirer.new('production'))
	get :index, params: {project_type: 'applab', limit: 1}

I would say that we have relatively high code coverage by unit tests here without a clear path to getting it to the 100% mark.

A UI test to catch this would load "more" applab projects and then scroll to the bottom of the screen. this would require creating 100+ projects during a UI test (or stubbing out the limit value passed by the client), which is not ideal.

A UI test could load the url like https://studio.code.org/api/v1/projects/gallery/public/applab/100 , as part of the UI test that publishes/features 1 project, and ensure that something shows up. that seems a bit hacky though.

Please let me know if you see a better way to regression test this feature.

[islemaster]

Understood. Thanks!