This is redundant checking. The route requires /:collection_id so this will always be present. It should suffice that we check if collection.nil? instead.

That sounds true, but what are we doing differently with a collection_slug parameter versus the usual collection_id (which should populate @collection?

This is one of the status typo. We are not interpolating the string here due to single quotes

Let's fix it!

Took the chance to add a temporary solution to one of my minor annoyances with Factories when combined with transactional_fixtures = false specs.

The issue is, doing just sequence(:display_name) { |n| "admin_#{n}_login" } without resetting the database each time we run specs IS FAILING at times, due to n here not accounting for duplicates (since each specs run does not delete the created data necessarily)

Using SecureRandom should work here. Initially I planned to add this to the already defined factory bots above, but it caused some legacy specs (especially Devise specs which needed to query created user by user_#{index}_login). So I decided we should add a sub-factory here :unique_user so it will always create a unique user regardless if we reset test db or not.

This should test each api route's if branches making sure everything works correctly.

Although, we are technically using factory bulk_export records here and not really checking the correct data processed (focusing only on response statuses)

I think it should be sufficient for the controller tests though. Given that this seems to be doing a lot of repetitive logic with the other bulk_controller, I am planning on writing a lib so that we just call that in the controller instead of doing the business logic in this controller (and then add the thorough data processed testing there)

I'm still surprised that @collection isn't automatically loaded here from the ApplicationController before filter. Can we check to see if it is and save a line of code?

We only have :set_api_user in the filter. We also expect :collection_slug instead of :collection_id so it won't be set using our before filters.

I think what we have now is ideal. We want to throw the message render status: 404, json: "No collection exists with the slug #{collection_slug}" since this is an API endpoint rather than a webpage. This allows user to know the error in their api calls.

You've convinced me.

Can you remind me what the precedence is of ! versus .blank? ? I'm trying to figure out why we added parentheses here.

blank? function produces boolean. We just invert the boolean value here.

Ideally I'd like to change this to .present? instead so we don't need to invert. But I kept this similar to old implementation as I may have thought this is a preferential thing. Do you want me to change it to present?

Sure! I wasn't aware that present? existed universally.