Label override flags for dbAuth generator #6440

Josh-Walker-GM · 2022-09-22T20:54:21Z

Resolves #3555

Description
Adds optional flags username-label and password-label which set the form labels in the SignupPage, LoginPage, ForgotPasswordPage and ResetPasswordPage generated via the yarn rw g dbAuth command.

Adds prompts to get these labels from the user if not already passed in by the flags. Prompt default values are Username and Password

Example of the CLI prompting:

These label values are also used to determine the name of the variables within the generated files. This means if you request that username-label be email then variables like usernameRef become emailRef. Thanks to @Tobbe for this good idea!

Outstanding

Snapshot tests to confirm correct labels
Update docs

Josh-Walker-GM · 2022-09-22T21:20:51Z

@cannikin In principle this meets the requirements of the initial #3555 description.

However, if it's okay I would ask what you and others may think regarding the following points:

Error messages returned from the backend and shown to users via the toast still contain references to the default values username and password. In principle this means this generator is half the work given it alters labels but not the toasts generated from the backend error messages. I don't see an easy way to update those values at all.
The additional check added in b793aed is needed solely for the existing tests. They don't experience yargs so never get a default value which is caught by the additional ternary. Is this check overkill if there is a nice way to provide the tests the default value?
Extracting out into a helper function the ability to generate lowercase, uppercase and capitalised versions of a command argument might be useful for tidying up the code here and in future for similar features?
Should we have tests to check the correctness of these label overrides?
In principle the label names could be determined via a prompt instead of command line flags. Depends what the best experience is?

Tobbe

Thanks for getting started on this PR. I left a few comments/suggestions. You're more than welcome to disagree with my suggestions 🙂 Just please tell me why in that case.

packages/cli/src/commands/generate/dbAuth/dbAuth.js

Co-authored-by: Tobbe Lundberg <[email protected]>

Josh-Walker-GM · 2022-09-23T09:05:40Z

Thanks @Tobbe, your suggestions were much better and I think the ?. approach is good.

cannikin · 2022-09-23T17:40:44Z

Another PR already?? You're in the zone now! 🙂

I do worry a bit that people will confuse setting these two labels with the actual names of the fields in the database. You're already free to make those whatever you want, you just have to let Redwood know what you called them in api/src/lib/auth.js. This change here is specifically for the generated pages, but the api side code (created with yarn rw setup auth dbAuth) is what contains the error messages shown in the toasts (more on that in #1 below). Likewise, the text labels should be regular, human readable words "Email Address" while the field names need to follow Prisma field naming conventions like "emailAddress". However, if a dev did enter emailAddress as the label for username, I believe using the title-case package would take care of turning that into Email Address (see #3 below).

Error messages returned from the backend and shown to users via the toast still contain references to the default values username and password. In principle this means this generator is half the work given it alters labels but not the toasts generated from the backend error messages. I don't see an easy way to update those values at all.

We could provide the same flags/prompts for the yarn rw setup auth dbAuth command. Again, worried that people will be confused between the text labels and the database field names. We'd just have to make it clear in the docs/prompt that this is for error message text only...

The additional check added in b793aed is needed solely for the existing tests. They don't experience yargs so never get a default value which is caught by the additional ternary. Is this check overkill if there is a nice way to provide the tests the default value?

You could put the default in a constant that's exported, and then check in the tests that the generated files contain that same value? See #4. I'd probably just call files() with the values you need as arguments, rather than worry about making yargs actually parse them and pull apart all the individual variables (we trust that yargs will do its job!). If you want to test that yargs is expecting the new flags, maybe there's a way to test the output of the builder() function and that the right list of commands has been appended somehow?

Extracting out into a helper function the ability to generate lowercase, uppercase and capitalised versions of a command argument might be useful for tidying up the code here and in future for similar features?

I was actually going to suggest you use the title-case package which we use in a couple other places. It'll take care of properly capitalizing multiple words, like Email Address. The usage of this is so minor we usually haven't extracted them out to separate functions: refactoring into a custom function instead of just using titleCase() from title-case seemed like overkill!

Should we have tests to check the correctness of these label overrides?

We've got a bunch of snapshot tests that verify a file is generated with the correct content. It doesn't look like we're using them in the generate dbAuth tests, but we could add them. Check out the tests for the dataMigration command, that has a couple at the end.

In principle the label names could be determined via a prompt instead of command line flags. Depends what the best experience is?

The best experience is probably to prompt...most people don't read the docs and would never even know it's an option. If you hit ENTER at each you get the defaults Username and Password.

dac09 · 2022-09-23T17:42:37Z

Wrong PR for the comment

Josh-Walker-GM · 2022-09-23T19:12:04Z

Thanks @cannikin, some thoughts in reply:

We could provide the same flags/prompts for the yarn rw setup auth dbAuth command. Again, worried that people will be confused between the text labels and the database field names. We'd just have to make it clear in the docs/prompt that this is for error message text only...

I agree if the web generators have the flags/prompts then the api side generators ought to have them too.
With regards to confusing people then I think we could simply add "label" to the flag name making it easier to distinguish its purpose?
This makes me wonder about some sort of meta-command which would take these sort of flags and then pass then down and run both web and api side generator commands - probably getting ahead of myself here but just what I'd think as a user.

You could put the default in a constant that's exported, and then check in the tests that the generated files contain that same value? See #4. I'd probably just call files() with the values you need as arguments, rather than worry about making yargs actually parse them and pull apart all the individual variables (we trust that yargs will do its job!). If you want to test that yargs is expecting the new flags, maybe there's a way to test the output of the builder() function and that the right list of commands has been appended somehow?

Hmm okay I'll probably need some time to mentally digest this one. In the mean time I'll leave the undefined default checks in as they can't do any harm.

I was actually going to suggest you use the title-case package which we use in a couple other places. It'll take care of properly capitalizing multiple words, like Email Address. The usage of this is so minor we usually haven't extracted them out to separate functions: refactoring into a custom function instead of just using titleCase() from title-case seemed like overkill!

I seen usage of this package within the code but wasn't immediately aware it was useful for this particular case - thanks for clarifying. I'll update to use that then for consistency. Definitely not looking to reinvent any wheels here!

We've got a bunch of snapshot tests that verify a file is generated with the correct content. It doesn't look like we're using them in the generate dbAuth tests, but we could add them. Check out the tests for the dataMigration command, that has a couple at the end.

I'll have a peak at the dataMigration related tests. As long as no one objects I'd probably add the tests in just to be sure.

The best experience is probably to prompt...most people don't read the docs and would never even know it's an option. If you hit ENTER at each you get the defaults Username and Password.

I have a slight affinity to prompts in that they actually prompt me to reconsider what to do when I'm running the command and also means I can't missing anything in the docs as I'm being spoon-fed them via the prompting. However, I understand prompts can be considered obtrusive and annoying.
This however raises an issue I'm running into Setting query name in cell generator #6442 where the prompting doesn't play nice with the current CLI. I see that Listr is used for the pretty CLI but then it appears quite difficult to do user input - at least the docs aren't too helpful and I've not been able to see an example in this codebase. This lead me to discover Listr2 which does cover user input in its own docs but then I see this uses Enquirer for prompting rather than the currently used prompts - this means lots of changes which may not be desirable or clash with any of your overarching CLI goals for the future.

In general I'd love to get a better idea of what direction you're all moving the CLI in to ensure the whole system is as homogenous as possible with any contributions I do try to make.

Tobbe · 2022-09-23T19:25:06Z

So the first thing you'd run as a user is yarn rw setup auth dbAuth, right? Let's say we add prompts there.

Could yarn rw g dbAuth then pick up on what options were selected in the auth setup prompts? Like make the dbAuth generator read the file(s) where the error messages are written and parse out the labels to use on the front-end to match. It does feel like we're maybe making the dbauth generator too tightly coupled to implementation details of the auth setup command, but on the other hand it feels silly to ask the user to basically answer the same question(s) twice.

Tobbe · 2022-09-23T19:29:42Z

In general I'd love to get a better idea of what direction you're all moving the CLI in to ensure the whole system is as homogenous as possible with any contributions I do try to make.

I'd recommend you read this issue thread. It's mostly about the initial yarn create redwood-app experience, but it does touch on some broader topics as well that are worth reading. #3989

Josh-Walker-GM · 2022-09-23T19:42:03Z

I'd recommend you read this issue thread. It's mostly about the initial yarn create redwood-app experience, but it does touch on some broader topics as well that are worth reading. #3989
Thanks, I'll have a read.

So the first thing you'd run as a user is yarn rw setup auth dbAuth, right? Let's say we add prompts there.

I agree so this would make the users workflow be something like:

Run yarn rw setup auth dbAuth
1. Prompt for username label - defaults to username
2. Prompt for password label - defaults to password
Run yarn rw g dbAuth
1. Parse yarn rw setup auth dbAuth files to detect labels used in 1.
2. Prompt to confirm these labels - defaults to true. If true then generate using these detected labels. If false prompt as in 1. to get what other labels to use instead.

How does that seem? People against the use of prompts will probably cry with that suggestion.

Tobbe · 2022-09-23T19:45:25Z

How does that seem?

I liked it all the way up to the second set of prompts 😂 When would one ever want to select something different there? Give me a good usecase and I'm easily convinced those prompts are a good idea too :)

cannikin · 2022-09-23T19:46:53Z

I originally split the two commands, thinking that 50% of people would want the api-side of dbAuth, but completely create the login pages from scratch, and the other 50% would just want to use our generated ones and customize them.

At this point, I wouldn't be surprised if 99% of people just run the generate pages command right after the setup command anyway. If we're adding prompts for the labels, we could just prompt Do you also want to generate the Login/Signup pages? at the end of yarn rw setup auth dbAuth and then just run the second command for them, forwarding the flags and prompt choices from the first.

If they don't, they can run the generate command manually, and get prompted again over there. 99% of people will never see these prompts a second time, and saves us the work of having to parse through the existing setup output to try and determine what the labels were, and applying them automatically.

Josh-Walker-GM · 2022-09-23T19:57:03Z

I think this leaves us with a possible consensus of both generators have prompts for labels and yarn rw setup auth dbAuth ends in a prompt asking if you want it to run yarn rw g dbAuth. This would forward the label choice silently on preventing excessive prompts but leaving users who run them sequentially with all the same options.

Only thing blocking that is getting prompts to work nicely 😂

Tobbe · 2022-09-23T20:02:30Z

You might not know this yet @Josh-Walker-GM, but we're rebuilding our whole auth subsystem, including setup commands. Right now dbAuth is getting some special treatment in our code, but that won't be the case moving forward. So whatever extra capabilities we give to dbAuth setup, we also must give (the possibility of) to other auth setup commands. So when you have a solution in mind that you think might work, also take a look at this PR #5985 and see how you could integrate the same solution there.

Josh-Walker-GM · 2022-09-23T20:16:56Z

@Tobbe Okay I'll bear this in mind. Can I ask where is best to ask/discuss things related to the current CLI tools - Listr, prompt etc?

Tobbe · 2022-09-23T20:31:33Z

Can I ask where is best to ask/discuss things related to the current CLI tools - Listr, prompt etc?

It depends... 😁 (Doesn't it always?)
If you have a well formed idea/opinion and some ideas on how to implement it: Create an RFC Issue here on GitHub
Just want to ask some questions and/or float some half-baked ideas you have? Feel free to post a comment in the issue I linked earlier #3989
Want to get the RW community's input (and also core team members): Post something on our forums (https://community.redwoodjs.com/)
I'm also up for a call if you want. But should probably try to get some more core team members join in on that call in that case.

Tobbe · 2022-09-23T20:35:52Z

You're also always free to join one of our contributors meetups and talk about your idea there. The next one is Oct 6th (unfortunately I won't be able to make that one)

https://community.redwoodjs.com/t/contributors-meetups-a-next-step-for-redwoodjs-participation/2470

Josh-Walker-GM · 2022-09-23T22:50:16Z

I'll put this PR on hold until the CLI repercussions brought about by #6442 are clear.

Josh-Walker-GM · 2022-09-30T21:08:29Z

Given this is going to rely on prompting as a core feature my plan is to wait for #6444 before continuing.

Tobbe · 2022-11-14T08:29:02Z

packages/cli/src/commands/generate/dbAuth/templates/forgotPassword.tsx.template

  useEffect(() => {
-    usernameRef?.current?.focus()
+    ${usernameCamelCase}Ref?.current?.focus()
  }, [])

  const onSubmit = async (data: { username: string }) => {


I think we could do ${usernameCamelCase} here, right? (And of course also when we pass it to forgotPassword)

Yeah I totally agree. I've implemented changes along these lines in d79294a.

Tobbe · 2022-11-14T18:50:16Z

I've updated to rename the ref variables to match the preferred labels. I didn't want to update or alias any of the function names or parameters - may end up being more confusing than helpful.

Hmm, maybe we should take an all-or-nothing approach here then? And just leave all of the code alone and only update the text as you had it initially. But I'm not convinced { username: email } is confusing. Also not convinced it isn't :D So let's think about it some more.

Next step after this PR is to do something similar for setup auth dbAuth, right? Would that update the db field name too?

I also experimented with adding in a conditional jsx comment to warn the user the api expects "username" and "password" but I couldn't get it to work.

Knee-jerk reaction is that it should be doable. Do you want to spend more time one trying to figure it out? If you do, you could share what you've tried and I we can both look at it

Josh-Walker-GM · 2022-11-14T22:30:47Z

Hmm, maybe we should take an all-or-nothing approach here then? And just leave all of the code alone and only update the text as you had it initially. But I'm not convinced { username: email } is confusing. Also not convinced it isn't :D So let's think about it some more.

Okay I'll spend a little more time on updating the code changes too then.

Next step after this PR is to do something similar for setup auth dbAuth, right? Would that update the db field name too?

You made the good point a while back that doing this would probably require the new packages which provide auth support to take the same options as the redwood dbAuth one does. This would be an unwanted requirement on these packages? Would we be okay with only the redwood dbAuth setup supporting such options? Definitely some thought needed for changes to that side.

Knee-jerk reaction is that it should be doable. Do you want to spend more time one trying to figure it out? If you do, you could share what you've tried and I we can both look at it

Your second point makes me realise we would only want this comment to be added based on the state of the API. Say a user already updates the API first and then goes to do the UI using the generators these comments would be incorrect. Perhaps it's better to simply not have them? EDIT: Also having code like {username: email, password: secret} should hopefully show what fields the API needs clearly enough.

Josh-Walker-GM · 2022-11-17T01:21:42Z

Just for my own sanity I went back and checked the changes to the docs. They are still correctly updated in my opinion. I don't think we need to specifically mention anything about the variables getting names which are consistent with the labels.

Thanks again @Tobbe for the suggestion to rename the variables, a big improvement I think.

Tobbe · 2022-11-24T07:00:30Z

Would we be okay with only the redwood dbAuth setup supporting such options

Definitely fine for only dbAuth to support this, as long as it's possible to add support to other providers too in the future if there's any appetite for it

Josh-Walker-GM · 2022-11-24T12:39:02Z

Okay I think perhaps it's best to add a small note at the end of the generator to warn users they will still have to update some of their api side code in order to get stuff like the error toasts to reflect their choices. Edit: Changed my mind I think we should leave the notice as is.

After that I think this is good?

cannikin · 2022-12-01T22:15:49Z

Okay ready to look this thing over! What state are we in, is this still good? Works with the decoupled auth updates?

Josh-Walker-GM · 2022-12-01T22:18:26Z

Not 100% confident on the decoupled auth stuff, I'll do a quick test on the latest canary to check it all still works. Other than that I think I'm happy with it.

EDIT: Hmm I tried running through the auth tutorial section on the latest version and things were broken. Blank pages with a Uncaught TypeError: (0 , _auth.createAuthentication) is not a function. I don't have much of a working understanding of the auth changes at the moment since they've been in flux. Ignore that... there was a slight issue in my local setup. It all works on the latest canary with my testing.

* initial ability to pass label arguements * Added remaining parameters to auth template files * Added no default value check in order to pass tests * Apply suggestions: reduce templateVars duplication Co-authored-by: Tobbe Lundberg <[email protected]> * Apply suggestions: replace ternary Co-authored-by: Tobbe Lundberg <[email protected]> * Added missing templateVars declaration * Renamed template vars * Renamed flags to include 'label', added prompts and used listr2 tasks for user interrogation * Updated yarn.lock * Added default values within function * Updated web authn page template - not tested * Moved WebAuthn into listr * Some tidying up * Fix default webauthn flag value check * Added enquirer option and other minor fixes * Added tests for prompts * Add web.app to the mocked getPaths * Fix password label and add listr2 options to task handler * Added snapshot tests * Added --username-label and --password-label flags to the docs for rw g dbAuth * Normalise paths within tests * Added test for custom labels and webauthn enabled * Add flags to prevent test project creation prompting the user * Updated test project fixtures * update snapshots and set mockfiles before test describe * Customised auth variables similar to labels * updated test fixtures * Update test snapshots * Updated to use customised variable names based on label option * Add missing dbAuth command test Co-authored-by: Tobbe Lundberg <[email protected]> Co-authored-by: Rob Cameron <[email protected]>

….com:redwoodjs/redwood into feat/dc-kc-decoupled-auth-setup-improvements * 'feat/dc-kc-decoupled-auth-setup-improvements' of github.com:redwoodjs/redwood: chore(deps): update dependency nx to v15.3.2 (#7114) chore(deps): update dependency redis to v4.5.1 (#7115) fix: add missing deps to cli helpers (#7117) Adds ability to delete a cache entry (#7016) Label override flags for dbAuth generator (#6440) fix(deps): update dependency systeminformation to v5.16.6 (#7108) feat: Generator rollbacks (#6947) fix(deps): update dependency @types/node to v16.18.8 (#7107) chore(deps): update dependency supertokens-node to v12.1.3 (#7105)

Josh-Walker-GM added 2 commits September 22, 2022 14:59

initial ability to pass label arguements

bf85e88

Added remaining parameters to auth template files

e6c3249

redwoodjs-bot bot assigned dac09 Sep 22, 2022

Added no default value check in order to pass tests

b793aed

Merge branch 'main' into dbAuth-labels-flag

2dbc0ad

dac09 assigned cannikin and unassigned dac09 Sep 22, 2022

Tobbe reviewed Sep 23, 2022

View reviewed changes

Josh-Walker-GM and others added 3 commits September 23, 2022 09:56

Apply suggestions: reduce templateVars duplication

f8350b0

Co-authored-by: Tobbe Lundberg <[email protected]>

Apply suggestions: replace ternary

a2644be

Co-authored-by: Tobbe Lundberg <[email protected]>

Added missing templateVars declaration

0ef86c2

Josh-Walker-GM added 2 commits October 3, 2022 22:14

Merge remote-tracking branch 'upstream/main' into dbAuth-labels-flag

4c1d50f

Renamed template vars

9c952a8

Update test snapshots

763bcc7

Tobbe reviewed Nov 14, 2022

View reviewed changes

Josh-Walker-GM added 2 commits November 14, 2022 23:08

Updated to use customised variable names based on label option

d79294a

Add missing dbAuth command test

3e60e0f

cannikin and others added 4 commits November 17, 2022 09:13

Merge branch 'main' into dbAuth-labels-flag

c2bb55d

Merge branch 'main' into dbAuth-labels-flag

3ddb62a

Merge remote-tracking branch 'upstream/main' into dbAuth-labels-flag

be6e9f8

Merge branch 'main' into dbAuth-labels-flag

bcc2759

Merge branch 'main' into dbAuth-labels-flag

31b8966

Tobbe and others added 2 commits November 24, 2022 16:56

Merge branch 'main' into dbAuth-labels-flag

846a2a7

Merge branch 'main' into dbAuth-labels-flag

fbd1f80

Josh-Walker-GM requested a review from Tobbe December 1, 2022 22:47

Josh-Walker-GM and others added 2 commits December 5, 2022 19:48

Merge branch 'main' into dbAuth-labels-flag

8d83aee

Merge branch 'main' into dbAuth-labels-flag

0a387ac

cannikin merged commit 21b6b51 into redwoodjs:main Dec 12, 2022

redwoodjs-bot bot added this to the next-release milestone Dec 12, 2022

Josh-Walker-GM deleted the dbAuth-labels-flag branch December 12, 2022 21:16

Tobbe mentioned this pull request Dec 13, 2022

Codemod to include full-name in test-project signup #7124

Merged

jtoar modified the milestones: next-release, v3.8.0 Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Label override flags for dbAuth generator #6440

Label override flags for dbAuth generator #6440

Josh-Walker-GM commented Sep 22, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 22, 2022 •

edited

Loading

Tobbe left a comment

Josh-Walker-GM commented Sep 23, 2022

cannikin commented Sep 23, 2022 •

edited

Loading

dac09 commented Sep 23, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022

Tobbe commented Sep 23, 2022

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022

cannikin commented Sep 23, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 23, 2022 •

edited

Loading

Tobbe commented Sep 23, 2022

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022 •

edited

Loading

Tobbe commented Sep 23, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 23, 2022

Josh-Walker-GM commented Sep 30, 2022

Tobbe Nov 14, 2022 •

edited

Loading

Josh-Walker-GM Nov 17, 2022

Tobbe commented Nov 14, 2022

Josh-Walker-GM commented Nov 14, 2022 •

edited

Loading

Josh-Walker-GM commented Nov 17, 2022

Tobbe commented Nov 24, 2022

Josh-Walker-GM commented Nov 24, 2022 •

edited

Loading

cannikin commented Dec 1, 2022

Josh-Walker-GM commented Dec 1, 2022 •

edited

Loading

Label override flags for dbAuth generator #6440

Label override flags for dbAuth generator #6440

Conversation

Josh-Walker-GM commented Sep 22, 2022 • edited Loading

Josh-Walker-GM commented Sep 22, 2022 • edited Loading

Tobbe left a comment

Choose a reason for hiding this comment

Josh-Walker-GM commented Sep 23, 2022

cannikin commented Sep 23, 2022 • edited Loading

dac09 commented Sep 23, 2022 • edited Loading

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022

Tobbe commented Sep 23, 2022

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022

cannikin commented Sep 23, 2022 • edited Loading

Josh-Walker-GM commented Sep 23, 2022 • edited Loading

Tobbe commented Sep 23, 2022

Josh-Walker-GM commented Sep 23, 2022

Tobbe commented Sep 23, 2022 • edited Loading

Tobbe commented Sep 23, 2022 • edited Loading

Josh-Walker-GM commented Sep 23, 2022

Josh-Walker-GM commented Sep 30, 2022

Tobbe Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Josh-Walker-GM Nov 17, 2022

Choose a reason for hiding this comment

Tobbe commented Nov 14, 2022

Josh-Walker-GM commented Nov 14, 2022 • edited Loading

Josh-Walker-GM commented Nov 17, 2022

Tobbe commented Nov 24, 2022

Josh-Walker-GM commented Nov 24, 2022 • edited Loading

cannikin commented Dec 1, 2022

Josh-Walker-GM commented Dec 1, 2022 • edited Loading

Josh-Walker-GM commented Sep 22, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 22, 2022 •

edited

Loading

cannikin commented Sep 23, 2022 •

edited

Loading

dac09 commented Sep 23, 2022 •

edited

Loading

cannikin commented Sep 23, 2022 •

edited

Loading

Josh-Walker-GM commented Sep 23, 2022 •

edited

Loading

Tobbe commented Sep 23, 2022 •

edited

Loading

Tobbe commented Sep 23, 2022 •

edited

Loading

Tobbe Nov 14, 2022 •

edited

Loading

Josh-Walker-GM commented Nov 14, 2022 •

edited

Loading

Josh-Walker-GM commented Nov 24, 2022 •

edited

Loading

Josh-Walker-GM commented Dec 1, 2022 •

edited

Loading