Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Add back and correct "resendoauthverification" command #779

Merged

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Jun 30, 2022

Fixes #770 by @zackkrida

Description

Corrects the email. I took a shot at the text of the corrected email but please feel free to offer better wording if you can think of any!

Testing instructions

Follow instructions from the original PR for testing locally: #694

Note also that CI should pass.

@sarayourfriend sarayourfriend requested a review from a team as a code owner June 30, 2022 05:49
@openverse-bot openverse-bot added 📄 aspect: text Concerns the textual material in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents labels Jun 30, 2022
@github-actions
Copy link

github-actions bot commented Jun 30, 2022

API Developer Docs Preview: Ready

https://wordpress.github.io/openverse-api/_preview/779

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

@sarayourfriend sarayourfriend changed the title Revert "Remove one-time-use command" Add back and correct "resendoauthverification" command Jun 30, 2022
@sarayourfriend sarayourfriend requested review from krysal and removed request for AetherUnbound June 30, 2022 06:04
@sarayourfriend sarayourfriend force-pushed the revert-768-remove/resend-oauth-verification-command branch from 7094db5 to 8e2d161 Compare June 30, 2022 06:05
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The copy is good, thank you.

@zackkrida
Copy link
Member

@obulat and @krysal can you review this today or tomorrow so we can resend these emails this week?

Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the dry version first and then the one with the --no-dry_run flag and I got different outputs. Is this expected?

Running just dj resendoauthverification:

The following applications had email verifications sent.                                                                                              

Application name: My test project
Cleaned related application count: 3
Cleaned related verification count: 3
Cleaned related registration count: 3


Application name: My test project2
Cleaned related application count: 0
Cleaned related verification count: 0
Cleaned related registration count: 0


Application name: My test project3
Cleaned related application count: 1
Cleaned related verification count: 1
Cleaned related registration count: 1

The above was a dry run and no records were actually affected.

Running just dj resendoauthverification --no-dry_run:

The following applications had email verifications sent.                                                                                              

Application name: My test project
Cleaned related application count: 3
Cleaned related verification count: 0
Cleaned related registration count: 3


Application name: My test project2
Cleaned related application count: 0
Cleaned related verification count: 0
Cleaned related registration count: 0


Application name: My test project3
Cleaned related application count: 1
Cleaned related verification count: 0
Cleaned related registration count: 1

I verified the link sent in the email and it works. It would be nice to see a unit test for it since the actual error was there. I'm less concerned as this has been already run in production, so approving to unblock.

@sarayourfriend
Copy link
Contributor Author

I do not know why that discrepancy exists. Reading through the code it doesn't seem like it should, the counts for each thing happen independent of the deletions and deleting one thing or another doesn't make the verifications query work different, at least not in any way that is evident to me by reading the code...

It would be nice to see a unit test for it since the actual error was there

I started to add a unit test that actually made a request with the URL... but I wasn't sure how to do that without having to spin up an API 🤔 If we use the RequestFactory approach it wouldn't really be all that useful, I don't think. We could assert in a regex or something that there are no double /s. Would that work for you?

@krysal
Copy link
Member

krysal commented Jul 6, 2022

We could assert in a regex or something that there are no double /s. Would that work for you?

That sounds good!

@sarayourfriend sarayourfriend force-pushed the revert-768-remove/resend-oauth-verification-command branch from ae17d9e to c645f74 Compare July 6, 2022 18:17
@sarayourfriend sarayourfriend merged commit b7765c4 into main Jul 6, 2022
@sarayourfriend sarayourfriend deleted the revert-768-remove/resend-oauth-verification-command branch July 6, 2022 18:27
@sarayourfriend
Copy link
Contributor Author

Added the unit test. If you put back the extra / they will fail. Thanks for the suggestion, Krystle!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📄 aspect: text Concerns the textual material in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sent API registration emails contained a typo
4 participants