-
Notifications
You must be signed in to change notification settings - Fork 21
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
Clarify local development login flow #2540
Changes from 9 commits
cc99edf
6407912
dbaa0f2
ec92165
63ed516
0cd53e4
e3ee1af
1bfb6b8
6d7c7dd
60a90ba
3d77aaa
e5ba667
83f8462
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -158,7 +158,7 @@ describe('`/api/users` endpoint', () => { | |||||||||||||
const response = await fetchApi(`/users`, agencies.own, fetchOptions.admin); | ||||||||||||||
expect(response.statusText).to.equal('OK'); | ||||||||||||||
const json = await response.json(); | ||||||||||||||
expect(json.length).to.equal(12); | ||||||||||||||
expect(json.length).to.equal(14); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like ideally these tests would be independent of the seed data. Does that track? (Regardless, seems outside the scope of this change, so I'm just updating the test to pass.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Maybe something like this instead?
Suggested change
Note: To make the above work, you'll also have to modify line 3 to import const { getSessionCookie, makeTestServer, knex } = require('./utils'); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this seems like a fair approach for now! (I'm curious, have you explored using synthetic test data generated by the tests themselves, as opposed to using common seed data? Any docs/discussions I can read up on?) |
||||||||||||||
}); | ||||||||||||||
it('lists users for an agency outside this user\'s hierarchy but in the same tenant', async () => { | ||||||||||||||
const response = await fetchApi(`/users`, agencies.offLimits, fetchOptions.admin); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,11 @@ function sendPassCode(email, passcode, httpOrigin, redirectTo) { | |
); | ||
|
||
if (process.env.DEV_LOGIN_LINK && process.env.NODE_ENV === 'development') { | ||
console.log(`Login link generated: \x1b[32m${href}`); | ||
const BLUE = '\x1b[34m'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if the preference was for something like this vs using a library like chalk. We seem to have chalk installed but only as a dev dependency, so I didn't want to rely on it here, but I'm happy to update if that feels cleaner. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 Given that this execution path is specific to dev workflows, is there a way to use chalk without declaring it as a production dependency? In general, my preference for logging is to slowly migrate over to structured logging (we currently use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible to use chalk here from devDependencies, but I think it ends up more messy than worth it. You run into a few issues you have to skirt:
Here's basically what that code would look like: if (process.env.DEV_LOGIN_LINK && process.env.NODE_ENV === 'development') {
// eslint-disable-next-line import/no-extraneous-dependencies
import('chalk').then((chalkModule) => {
const chalk = chalkModule.default;
const message = `| Login link generated: ${href} |`;
console.log(chalk.blue('-'.repeat(message.length)));
console.log(chalk.blue(message));
console.log(chalk.blue('-'.repeat(message.length)));
});
} Personally, I don't think it's a particular improvement. It also has the small danger of someone copy-pasting code that relies on chalk outside of the If this is the only instance of wanting color output for now, I'd be inclined to leave it as-is. If we start needing this a few places, I'd probably consider promoting chalk to a full dependency. Since this is server-side code, we shouldn't be as worried about bundle size, etc. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with the above – let's leave as-is unless/until there are any compelling use-cases for production. |
||
const message = `| Login link generated: ${href} |`; | ||
console.log(`${BLUE}${'-'.repeat(message.length)}`); | ||
console.log(`${BLUE}${message}`); | ||
console.log(`${BLUE}${'-'.repeat(message.length)}`); | ||
} | ||
return module.exports.deliverEmail({ | ||
toAddress: email, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️