Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

Fix: Binary string in email verification #1154

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

decon-harsh
Copy link
Member

The url's uid was a b string. Decoded the uid to string. Removed some flake8 errors.

Fixes #1061

Initial PR #1086

Type of Change:

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

Mocks

Screenshot from 2020-09-04 00-10-37

Screenshot from 2020-09-04 00-10-46

How Has This Been Tested?

Manually

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings

@decon-harsh
Copy link
Member Author

@devkapilbansal Please check this!

@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #1154 into develop will increase coverage by 0.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1154      +/-   ##
===========================================
+ Coverage    86.93%   87.18%   +0.24%     
===========================================
  Files           85       85              
  Lines         4057     4057              
  Branches       237      237              
===========================================
+ Hits          3527     3537      +10     
+ Misses         458      448      -10     
  Partials        72       72              
Impacted Files Coverage Δ
vms/registration/views.py 82.35% <0.00%> (+3.20%) ⬆️
vms/event/views.py 79.32% <0.00%> (+1.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb63dfd...266a634. Read the comment docs.

Copy link
Member

@devkapilbansal devkapilbansal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks @decon-harsh for your time.

The changes made in this PR were tested locally. Following are the results:

  1. Code review - Done

  2. All possible responses (positive and negative tests) were tested as below:

    • Test1 Description Tested email verification
      Screenshot/gif:
      test_result
  3. Additional Comments: As there was already a PR Bug: Fixed binary string in the url of email verification #1086 which was reviewed but messed by due to some changes, this PR can be labelled as Ready to Merge. Also, changes done in fix: Admin registration issue #1003 should be done first as there is a bug in registration too as witnessed in above screenshot.

  4. Status of PR can be Changed to: Ready to Merge.

@decon-harsh
Copy link
Member Author

Thanks for your precious time @devkapilbansal . I guess i have nothing to change in this PR now? .

Please keep this PR in mind while reviewing OSH works.

@devkapilbansal
Copy link
Member

Although this PR is same as the older one, but pinging @SanketDG @satya7289 @isabelcosta to look into this too

@isabelcosta
Copy link
Member

@devkapilbansal thank you for the ping!

@gaurivn gaurivn added the Status: Needs Review PR needs an additional review or a maintainer's review. label Oct 24, 2020
@decon-harsh
Copy link
Member Author

When will it get merged? @devkapilbansal

@devkapilbansal
Copy link
Member

@decon-harsh consider asking for review on Zulip. After two approved reviews and testing, this PR can be merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix binary string in the URL on the email verification
4 participants