Added ResendConfirmation GraphQL method. #35

aarona · 2019-10-31T05:37:33Z

No description provided.

mcelicalderon · 2019-10-31T15:50:45Z

Much better, thanks for fixing your commits. I will review both PRs as soon as I have some free time. Just a note on this one, it also contains the changes to the docs you included in the other PR. This one should ONLY be about the resendConfirmation mutation. No need to close the PRs, they can always be fixed.

aarona · 2019-11-01T00:24:23Z

Ok, got it! Yeah I also noticed that. A throwback from when I did my documentation work on master instead of its own branch. I tried fixing those PRs with a fresh copy from upstream and they didn't seem to work. I shouldn't have that issue again though now that I'm doing things correctly. Thanks for your patience and also thank you for giving me this task. It was fun to do and helped me learn a good position of the gem's code base. Much appreciated!

aarona · 2019-11-01T03:09:15Z

If you notice, I added documentation for the ResendConfirmation method. I saved this file so that only that change is there but I haven't pushed it to my github feature branch. After you review this code, do you want me to commit that change so that only the ResendConfirmation code is effected, or is this PR fine as is?

Updated documentation for the mutation and query graphql methods.

mcelicalderon · 2019-11-04T16:07:43Z

I merged the #34, you need to resolve conflicts on this branch because of that. I still have to review the resend confirmation mutation, but on the mean time you could cleanup the readme, it should also include docs for the new mutation (just add it to the list as the others)

…ql_devise into resend_confirmation Conflicts: README.md

mcelicalderon · 2019-11-04T18:02:47Z

Why did you close this one? Please don't open another one just because of git conflicts. Fix your branch locally and push to this same branch. you can push force git push -f when you have fixed your branch locally

aarona · 2019-11-04T18:07:04Z

Sorry after I updated my feature branch it looked like the other branch with a bunch of changes and I thought wouldn't want that so I was trying to fix it so it was a clean push. I'll fix this.

aarona · 2019-11-04T18:14:13Z

When I looked at the changes files, it had files that I didn't change (see https://github.com/graphql-devise/graphql_devise/pull/35/files). It looks like I'm trying to add the Issue configuration files but thats from another PR that someone else did.

aarona · 2019-11-04T18:39:59Z

Ok All the code in this PR is exactly what I want to add to the master branch.

mcelicalderon

Looks good! Thanks for the PR. Left some comments, let me know if you have questions.

app/graphql/graphql_devise/mutations/resend_confirmation.rb

config/locales/en.yml

spec/requests/mutations/resend_confirmation_spec.rb

00dav00

@aarona could you add test to know what would happen if the email provided belongs to a user that is already confirmed? How devise behaves in this case?

aarona · 2019-11-06T01:33:56Z

Yes, thats a really good idea and I was considering that. I wanted to ask you guys if you had any other scenarios to test for. I'm going to sit down tonight and work on these changes.

config/locales/en.yml

aarona · 2019-11-13T19:06:26Z

Just bumping this PR. I've made the changes you guys had requested.

mcelicalderon · 2019-11-13T19:35:39Z

Thanks! I will take a look ASAP

00dav00

@aarona left a couple comments for you, this is looking good :)

README.md

app/graphql/graphql_devise/mutations/resend_confirmation.rb

spec/requests/mutations/resend_confirmation_spec.rb

mcelicalderon

Looking great @aarona. Just some small changes to address, PR is getting there.

app/graphql/graphql_devise/mutations/resend_confirmation.rb

mcelicalderon · 2019-11-20T14:58:48Z

app/graphql/graphql_devise/mutations/resend_confirmation.rb

+        if resource
+          yield resource if block_given?
+
+          raise_user_error("Email #{I18n.t('errors.messages.already_confirmed')}") if resource.confirmed?


Why interpolate a localized string here? Just put the entire string in the locales file please

The reason why I did this is because it seemed you wanted to fall back on Devise locales. The errors.message.already_confirmed value is: "was already confirmed, please try signing in". I can make this whole thing as a locale entry but I'll have to put it into the graphql_devise locale. Would you prefer that instead?

yes please, move the entire message to our own locales file. I think the rule for localized messages should be that ANY message we use directly in OUR code, should go in our locales file. Otherwise we depend on the other gems to keep those keys on their locales files, if for any reason a version of those gems changes the messages, it could affect our gem. I'll have to check the rest of the code for the same thing, but for this PR, please make sure any localized message YOU use in the code has an entry in our locales file (the entire message always as some languages might have a completely different way to say something)

Yes check the /app/views/graphql_devise/mailer/reset_password_instructions.html.erb file because it also relies on Devise's locale file. I can create a seperate PR for that and fix that.

I'll fix mine and update this PR.

Actually it looks like both mailers do this. I'll create a PR for both later.

spec/requests/mutations/resend_confirmation_spec.rb

mcelicalderon

Looks good @aarona I think the locales thing is the most important one to take care of now, just make sure any message you are using comes from an entry on our locales file. Other small style comments and should be good to merge

spec/requests/mutations/resend_confirmation_spec.rb

mcelicalderon · 2019-11-22T14:40:53Z

app/graphql/graphql_devise/mutations/resend_confirmation.rb

+        if resource
+          yield resource if block_given?
+
+          raise_user_error("Email #{I18n.t('errors.messages.already_confirmed')}") if resource.confirmed?


yes please, move the entire message to our own locales file. I think the rule for localized messages should be that ANY message we use directly in OUR code, should go in our locales file. Otherwise we depend on the other gems to keep those keys on their locales files, if for any reason a version of those gems changes the messages, it could affect our gem. I'll have to check the rest of the code for the same thing, but for this PR, please make sure any localized message YOU use in the code has an entry in our locales file (the entire message always as some languages might have a completely different way to say something)

spec/requests/mutations/resend_confirmation_spec.rb

mcelicalderon

Good job, thanks! I will review all locales in the gem and then release a new version with your changes 🙌

aarona added 2 commits October 30, 2019 22:25

Updated documentation for the mutation and query graphql methods.

d6324c4

Added ResendConfirmation GraphQL method.

898a652

Fixed a misspelling

7e27c65

00dav00 and others added 2 commits November 4, 2019 06:35

Add github templates for issues (graphql-devise#36)

ac9c1ab

Merge pull request graphql-devise#34 from aarona/mut_que_docs

ee76d39

Updated documentation for the mutation and query graphql methods.

aarona added 4 commits November 4, 2019 09:43

Merged master into resend_confirmation.

1e09349

Added ResendConfirmation GraphQL method.

083570f

Merge branch 'resend_confirmation' of https://github.com/aarona/graph…

9ee3fbc

…ql_devise into resend_confirmation Conflicts: README.md

Pulled upstream master and then rebased master on send_confirmation

537e3f2

aarona closed this Nov 4, 2019

aarona deleted the resend_confirmation branch November 4, 2019 17:55

Pulled upstream master and rebased master in resend_confirmation

2236216

aarona reopened this Nov 4, 2019

aarona added 2 commits November 4, 2019 10:30

Removed rebased code to seperate the Resend Confirmation changes.

d1539bf

Removed rebased code to seperate the Resend Confirmation changes.

53e7bd4

mcelicalderon requested changes Nov 5, 2019

View reviewed changes

00dav00 reviewed Nov 6, 2019

View reviewed changes

mcelicalderon reviewed Nov 6, 2019

View reviewed changes

config/locales/en.yml Show resolved Hide resolved

mcelicalderon reviewed Nov 6, 2019

View reviewed changes

config/locales/en.yml Outdated Show resolved Hide resolved

Code review fixes and test enhancements per @00dav00's request.

0ced80c

00dav00 requested changes Nov 18, 2019

View reviewed changes

mcelicalderon mentioned this pull request Nov 20, 2019

Resend verification mail when an unverified account try to log in #40

Closed

mcelicalderon requested changes Nov 20, 2019

View reviewed changes

Code review requests per @mcelicalderon and @00dav00

4e83a8b

mcelicalderon requested changes Nov 22, 2019

View reviewed changes

aarona added 2 commits November 22, 2019 16:08

Moved reliance for Resend Instructions from Devise to Graphql Devise.

58014a4

Some RSpec styling changes per @mcelicalderon's request.

247819e

mcelicalderon approved these changes Nov 24, 2019

View reviewed changes

mcelicalderon merged commit 97d1c7e into graphql-devise:master Nov 24, 2019

mcelicalderon added the enhancement New feature or request label Nov 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ResendConfirmation GraphQL method. #35

Added ResendConfirmation GraphQL method. #35

aarona commented Oct 31, 2019

mcelicalderon commented Oct 31, 2019 •

edited

Loading

aarona commented Nov 1, 2019

aarona commented Nov 1, 2019

mcelicalderon commented Nov 4, 2019

mcelicalderon commented Nov 4, 2019

aarona commented Nov 4, 2019

aarona commented Nov 4, 2019

aarona commented Nov 4, 2019

mcelicalderon left a comment

00dav00 left a comment

aarona commented Nov 6, 2019

aarona commented Nov 13, 2019

mcelicalderon commented Nov 13, 2019

00dav00 left a comment

mcelicalderon left a comment

mcelicalderon Nov 20, 2019

aarona Nov 22, 2019

mcelicalderon Nov 22, 2019

aarona Nov 22, 2019

aarona Nov 22, 2019

mcelicalderon left a comment

mcelicalderon Nov 22, 2019

mcelicalderon left a comment

Added ResendConfirmation GraphQL method. #35

Added ResendConfirmation GraphQL method. #35

Conversation

aarona commented Oct 31, 2019

mcelicalderon commented Oct 31, 2019 • edited Loading

aarona commented Nov 1, 2019

aarona commented Nov 1, 2019

mcelicalderon commented Nov 4, 2019

mcelicalderon commented Nov 4, 2019

aarona commented Nov 4, 2019

aarona commented Nov 4, 2019

aarona commented Nov 4, 2019

mcelicalderon left a comment

Choose a reason for hiding this comment

00dav00 left a comment

Choose a reason for hiding this comment

aarona commented Nov 6, 2019

aarona commented Nov 13, 2019

mcelicalderon commented Nov 13, 2019

00dav00 left a comment

Choose a reason for hiding this comment

mcelicalderon left a comment

Choose a reason for hiding this comment

mcelicalderon Nov 20, 2019

Choose a reason for hiding this comment

aarona Nov 22, 2019

Choose a reason for hiding this comment

mcelicalderon Nov 22, 2019

Choose a reason for hiding this comment

aarona Nov 22, 2019

Choose a reason for hiding this comment

aarona Nov 22, 2019

Choose a reason for hiding this comment

mcelicalderon left a comment

Choose a reason for hiding this comment

mcelicalderon Nov 22, 2019

Choose a reason for hiding this comment

mcelicalderon left a comment

Choose a reason for hiding this comment

mcelicalderon commented Oct 31, 2019 •

edited

Loading