Free specs from seeding #927
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
solebared
force-pushed
the
free-specs-from-seeding
branch
from
e1504f8
to
c85f196
Compare
|
Love this: |
maebeale
approved these changes
Apr 8, 2021
h-m-m
reviewed
Apr 8, 2021
Comment on lines
+78
to
+79
| # There should always be a current org, but being defensive here helps simplify tests | ||
| Organization.instance_owner&.logo_url.presence || asset_pack_path('media/images/logo.png') |
There was a problem hiding this comment.
Thank you! I've tripped up on this many times, and this feels to me like a much better home for this conditional than anywhere else it has lived before
solebared
force-pushed
the
org-instance-owner
branch
from
9762e39
to
901a630
Compare
1. Move logo defaulting method out of Vue into the backend 2. Accommodate current_organization being nil: intended to support specs without having to seed the test db. 3. Extract helper method to consolidate defaulting logic Note: i don't love adding this to application_helper, particularly since that module is already quite cluttered. Couldn't come up with a better option though.
Allows simpler assertions than a request spec would. Also means the spec doesn't have to rely on a seeded test DB.
Inverting the dependency here allows specs to easily pass in a SystemSetting instance instead of relying on the test db to have been seeded.
Allows for organization to be injected in specs instead of having to mock Organization.instance_owner or relying on it having been seeded.
Allows specs to inject organization instead of having to rely on seeding or class method mocking.
solebared
force-pushed
the
free-specs-from-seeding
branch
from
c85f196
to
3539330
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Our ruby specs currently depend on the test database being seeded, which is non-standard and surprising, particularly for new contributors.
What, How
logo_urllogic into a new helper method and make it defensive in case the DB doesn't have aninstance_owner.shared/_organization_logoandNavBar.vue.GenerateLandingPageJsoninteractor out ofPublicPagesController.landing_pageto ease testing.LocationTypein a couple of specs instead of relying on one having been seeded.SystemSettingintoSubmissionMailerso it can be injected easily in specs.OrganizationintoEmailNewSubmissionandEmailPeerinteractors, and the corresponding mailers (SubmissionMailer,PeerToPeerMatchMailer)Testing
Relies on existing specs and some manual testing of nav bar, login and signup pages.
Next Steps
Organization.instance_ownershould live inContext, similar toSystemSetting.current_settings.Outstanding Questions, Concerns and Other Notes
Note this is built on top of #926 which renamed
Organization.current_organizationto.instance_owner. However the changes here aren't really dependent on that particular change.Pre-Merge Checklist