Skip to content
This repository has been archived by the owner on Aug 21, 2022. It is now read-only.

added custom fields to user model #69

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

ElijahAhianyo
Copy link
Collaborator

@ElijahAhianyo ElijahAhianyo commented Dec 18, 2020


This change is Reviewable

@gitpod-io
Copy link

gitpod-io bot commented Dec 18, 2020

Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

also write some unit tests creating and committing models with some edge cases for this model.
cases are when email is verified but mobile num is not and vice versa.
unless both of them are not verified user is not verified.
for that purpose, you could do something like:

class User:
  ..
  ..
  @property
  def is_verified(self):
    return self.otp_verified and self.email_verified

check again after running shell_plus or from DB model schema, i think its self.auth_user.email_verified ;

apps/auth_zero/migrations/0004_auto_20201218_1031.py Outdated Show resolved Hide resolved
apps/auth_zero/models.py Outdated Show resolved Hide resolved
apps/auth_zero/models.py Outdated Show resolved Hide resolved
apps/auth_zero/models.py Outdated Show resolved Hide resolved
@codecakes codecakes added the enhancement New feature or request label Dec 18, 2020
@codecakes
Copy link
Contributor

@ElijahAhianyo if working on JIRA tasks, use smart commits

@codecakes
Copy link
Contributor

@ElijahAhianyo first un apply this particular migration and then remigrate after making changes

@milan-tom
Copy link
Collaborator

milan-tom commented Dec 18, 2020

also write some unit tests creating and committing models with some edge cases for this model.
cases are when email is verified but mobile num is not and vice versa.
unless both of them are not verified user is not verified.
for that purpose, you could do something like:

class User:
  ..
  ..
  @property
  def is_verified(self):
    return self.otp_verified and self.email_verified

check again after running shell_plus or from DB model schema, i think its self.auth_user.email_verified ;

@ElijahAhianyo I can do this section. You can focus on the rest of the requested changes.

Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

would great if @milan-tom could merge his pr ElijahAhianyo#1 to your branch @ElijahAhianyo

covidX/settings.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 30, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.47 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@codecakes
Copy link
Contributor

DeepCode's analysis on #e8374e found:

* ℹ️ **2** minor issues. 👇

Top issues

Description Example fixes
Do not hardcode credentials in code. Found hardcoded credential used in username. Occurrences:

* [tests.py:11](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L11)

* [tests.py:21](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L21)

* [tests.py:32](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L32)

🔧 Example fixes
Do not hardcode passwords in code. Found hardcoded password used in password. Occurrences:

* [tests.py:13](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L13)

* [tests.py:23](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L23)

* [tests.py:34](https://github.com/Xcov19/covidX/blob/e8374e5299a0abc9636393f8968be082ecde90ab/apps/auth_zero/tests.py#L34)

🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@milan-tom do the following to fix the ☝️ issue and bring more structure and clarity to overall testing :

  • create config/init.py folder and move all credentials you are creating in a test_config.ini file
  • Define sections as follows which will help you organize test data:
[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 
  • you can use configparser built-in lib to load from here like:
import six
import configparser

confg = configparser.ConfigParser()
confg.read(your ini file)

user = six.text_type(confg['unverified']['USER']) and so on..

Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

@milan-tom left few comments. I've requested some changes. this will bring more structure as we bring test coverage to our codebase. i'll review again on those changes.

apps/auth_zero/tests.py Outdated Show resolved Hide resolved
apps/auth_zero/tests.py Outdated Show resolved Hide resolved
apps/auth_zero/tests.py Outdated Show resolved Hide resolved
apps/auth_zero/tests.py Outdated Show resolved Hide resolved
@milan-tom
Copy link
Collaborator

milan-tom commented Jan 1, 2021

@milan-tom do the following to fix the ☝️ issue and bring more structure and clarity to overall testing :

  • create config/init.py folder and move all credentials you are creating in a test_config.ini file
  • Define sections as follows which will help you organize test data:
[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 
  • you can use configparser built-in lib to load from here like:
import six
import configparser

confg = configparser.ConfigParser()
confg.read(your ini file)

user = six.text_type(confg['unverified']['USER']) and so on..

What is the purpose of six.text_type here @codecakes ?

@codecakes
Copy link
Contributor

@milan-tom do the following to fix the ☝️ issue and bring more structure and clarity to overall testing :

  • create config/init.py folder and move all credentials you are creating in a test_config.ini file
  • Define sections as follows which will help you organize test data:
[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 
  • you can use configparser built-in lib to load from here like:
import six
import configparser

confg = configparser.ConfigParser()
confg.read(your ini file)

user = six.text_type(confg['unverified']['USER']) and so on..

What is the purpose of six.text_type here @codecakes ?

Easy to google and understand

@milan-tom
Copy link
Collaborator

@milan-tom do the following to fix the ☝️ issue and bring more structure and clarity to overall testing :

  • create config/init.py folder and move all credentials you are creating in a test_config.ini file
  • Define sections as follows which will help you organize test data:
[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 
  • you can use configparser built-in lib to load from here like:
import six
import configparser

confg = configparser.ConfigParser()
confg.read(your ini file)

user = six.text_type(confg['unverified']['USER']) and so on..

What is the purpose of six.text_type here @codecakes ?

Easy to google and understand

I did Google it. However, I just don't understand why we need it when the values are converted to strings by default. I have made a solution that does not require it.

@codecakes
Copy link
Contributor

@milan-tom do the following to fix the ☝️ issue and bring more structure and clarity to overall testing :

  • create config/init.py folder and move all credentials you are creating in a test_config.ini file
  • Define sections as follows which will help you organize test data:
[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 
  • you can use configparser built-in lib to load from here like:
import six
import configparser

confg = configparser.ConfigParser()
confg.read(your ini file)

user = six.text_type(confg['unverified']['USER']) and so on..

What is the purpose of six.text_type here @codecakes ?

Easy to google and understand

I did Google it. However, I just don't understand why we need it when the values are converted to strings by default. I have made a solution that does not require it.

Because you are assuming a lot of things: the underlying code is guaranteed to be yours and it behaves a certain way.
This will likely change tomorrow and we might not use .ini. That's why this line in your code looks convenient to you.
What if the input was byte stream?

Use six.ensure_text to ensure it gets the text input. Break that line with correct interface definition like:

@dataclass(frozen=True)
class Credentials:
  user: str
  pass: str
  something_else: int

class CredentialsFactoryLoader:
  def __init__(self, config_file, credentials_class: credentials_class: TypeVar('Credentials', bound=True) = None, loader_class=None):
    self.credentials_class = credentials_class 
    self.loader = loader_class(config_file)

  def read_verified(self):
      return self.credentials_class(self.loader.read_verified_config())

  def read_unverified(self):
      return self.credentials_class(self.loader.read_unverified_config())  
 

class ConfigByteLoader():
  // some logic to load
  def sanitize(self):
    value = {}
     ensure whatever transformation you do here.
     ensure_text where text field expected
     ensure digits are converted
     return value dict here;
  def read_verified_config(self):
      pass

class ConfigFileLoader():
     def __init__(self, config_file: str):
       self.config_setting = config.read_file(config_file)
 
    def read_verified_config(self) -> dict:
      ensure whatever transformation you do here.
      ensure_text where text field expected
      ensure digits are converted
      return value dict here;
      return dict(user=self.config_settings['user'], pass=self.config_settings['pass'])
       

// older loader -> 
// creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigFileLoader) 
creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigByteLoader) 
create_user(**creds_obj.read_unverified())

Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

format files

apps/auth_zero/models.py Outdated Show resolved Hide resolved
apps/auth_zero/tests.py Outdated Show resolved Hide resolved
@milan-tom
Copy link
Collaborator

Because you are assuming a lot of things: the underlying code is guaranteed to be yours and it behaves a certain way.
This will likely change tomorrow and we might not use .ini. That's why this line in your code looks convenient to you.
What if the input was byte stream?

Use six.ensure_text to ensure it gets the text input. Break that line with correct interface definition like:

@dataclass(frozen=True)
class Credentials:
  user: str
  pass: str
  something_else: int

class CredentialsFactoryLoader:
  def __init__(self, config_file, credentials_class: credentials_class: TypeVar('Credentials', bound=True) = None, loader_class=None):
    self.credentials_class = credentials_class 
    self.loader = loader_class(config_file)

  def read_verified(self):
      return self.credentials_class(self.loader.read_verified_config())

  def read_unverified(self):
      return self.credentials_class(self.loader.read_unverified_config())  
 

class ConfigByteLoader():
  // some logic to load
  def sanitize(self):
    value = {}
     ensure whatever transformation you do here.
     ensure_text where text field expected
     ensure digits are converted
     return value dict here;
  def read_verified_config(self):
      pass

class ConfigFileLoader():
     def __init__(self, config_file: str):
       self.config_setting = config.read_file(config_file)
 
    def read_verified_config(self) -> dict:
      ensure whatever transformation you do here.
      ensure_text where text field expected
      ensure digits are converted
      return value dict here;
      return dict(user=self.config_settings['user'], pass=self.config_settings['pass'])
       

// older loader -> 
// creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigFileLoader) 
creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigByteLoader) 
create_user(**creds_obj.read_unverified())

Could I use django.utils.encoding.force_text for this?

@codecakes
Copy link
Contributor

Because you are assuming a lot of things: the underlying code is guaranteed to be yours and it behaves a certain way.
This will likely change tomorrow and we might not use .ini. That's why this line in your code looks convenient to you.
What if the input was byte stream?
Use six.ensure_text to ensure it gets the text input. Break that line with correct interface definition like:

@dataclass(frozen=True)
class Credentials:
  user: str
  pass: str
  something_else: int

class CredentialsFactoryLoader:
  def __init__(self, config_file, credentials_class: credentials_class: TypeVar('Credentials', bound=True) = None, loader_class=None):
    self.credentials_class = credentials_class 
    self.loader = loader_class(config_file)

  def read_verified(self):
      return self.credentials_class(self.loader.read_verified_config())

  def read_unverified(self):
      return self.credentials_class(self.loader.read_unverified_config())  
 

class ConfigByteLoader():
  // some logic to load
  def sanitize(self):
    value = {}
     ensure whatever transformation you do here.
     ensure_text where text field expected
     ensure digits are converted
     return value dict here;
  def read_verified_config(self):
      pass

class ConfigFileLoader():
     def __init__(self, config_file: str):
       self.config_setting = config.read_file(config_file)
 
    def read_verified_config(self) -> dict:
      ensure whatever transformation you do here.
      ensure_text where text field expected
      ensure digits are converted
      return value dict here;
      return dict(user=self.config_settings['user'], pass=self.config_settings['pass'])
       

// older loader -> 
// creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigFileLoader) 
creds_obj = CredentialsFactoryLoader(config_file, Credentials, ConfigByteLoader) 
create_user(**creds_obj.read_unverified())

Could I use django.utils.encoding.force_text for this?

Yes, either one. six is compatibility module which we're already using. regardless, define interface so that we know what interface to conform to if we ever change the underlying abstraction. most likely, we will modify the config file soon;

apps/auth_zero/tests.py Outdated Show resolved Hide resolved
common/config.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jan 16, 2021

This pull request introduces 2 alerts when merging 5a5471a into 1a1ed31 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for 'import *' may pollute namespace

@lgtm-com
Copy link

lgtm-com bot commented Jan 16, 2021

This pull request introduces 1 alert when merging 56096ab into 1a1ed31 - view on LGTM.com

new alerts:

  • 1 for 'import *' may pollute namespace

@milan-tom milan-tom requested a review from codecakes January 16, 2021 20:30
common/config.py Outdated Show resolved Hide resolved
@milan-tom milan-tom self-requested a review January 23, 2021 11:38
@lgtm-com
Copy link

lgtm-com bot commented Jan 23, 2021

This pull request introduces 1 alert when merging 8feef55 into df1fc21 - view on LGTM.com

new alerts:

  • 1 for 'import *' may pollute namespace

Copy link
Collaborator

@milan-tom milan-tom left a comment

Choose a reason for hiding this comment

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

All of @codecakes' requested changes have been completed. The only failing checks are due to the following reasons:

  • Formatting problems in some migrations ➔ Based upon my understanding, migrations shouldn't be edited manually
  • 1 blank line required before class docstring (found 0) (D203) error from Codacy Static Code Analysis ➔ PEP 257 has removed this recommendation and so I think this check should be removed
  • Wildcard import base ➔ This seems kind of unavoidable in this case as we want to build the dev settings file on top of the base settings file
  • No name 'config' in module 'common' ➔ Not sure why this error appears despite there being a __init__.py file in common
  • Unused variable 'acc_type_ids' error in apps/auth_zero/models.py ➔ Perhaps we should remove this variable although it is not part of this pr

covidX/settings/dev.py Outdated Show resolved Hide resolved
@marcelloromani
Copy link
Collaborator

PyCQA/pydocstyle#141

Looks like in order to pass Codacy Static Code Analysis D203 needs to be disabled.

@codecakes
Copy link
Contributor

I have merged my branch; there are changes that need to be merged back here from master

@codecakes
Copy link
Contributor

make sure apps.common.config absolute imports everywhere

Copy link
Collaborator

@milan-tom milan-tom left a comment

Choose a reason for hiding this comment

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

make sure apps.common.config absolute imports everywhere

Are there any remaining common.config imports?

@milan-tom milan-tom changed the base branch from master to develop March 21, 2021 20:30
@codecakes codecakes added help wanted Extra attention is needed needs-review labels Mar 27, 2021
@codecakes
Copy link
Contributor

make sure apps.common.config absolute imports everywhere

Are there any remaining common.config imports?

your code will throw errors on testing if there are any

@codecakes
Copy link
Contributor

Hi @ElijahAhianyo I tested the branch:

  • created private key and certificate:
openssl req -x509 -newkey rsa:4096 -keyout privateKey.key -out certificate -days 365 -nodes
  • created .env file based on values given by @codecakes privately
  • started the application:
docker-compose up --build
  • result: build errors:
(16:23:43) ERROR: /covidX/BUILD:16:11: Label '//covidX:settings/base.py' is invalid because 'covidX/settings' is a subpackage; perhaps you meant to put the colon here: '//covidX/settings:base.py'?
(16:23:43) ERROR: /BUILD:37:10: Target '//covidX:urls' contains an error and its package is in error and referenced by '//:manage'
(16:23:43) ERROR: /BUILD:37:10: Target '//covidX:asgi' contains an error and its package is in error and referenced by '//:manage'
(16:23:43) ERROR: /BUILD:37:10: Target '//covidX:wsgi' contains an error and its package is in error and referenced by '//:manage'
(16:23:43) ERROR: Analysis of target '//:manage' failed; build aborted: Analysis failed

@marcelloromani this should be resolved now @milan-tom ?

@gitpod-io
Copy link

gitpod-io bot commented May 1, 2021

@codecakes codecakes requested a review from milan-tom May 1, 2021 19:09
@gitpod-io
Copy link

gitpod-io bot commented May 2, 2021

@gitpod-io
Copy link

gitpod-io bot commented May 2, 2021

@gitpod-io
Copy link

gitpod-io bot commented May 3, 2021

@gitpod-io
Copy link

gitpod-io bot commented May 3, 2021

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Extra attention is needed needs-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants