Xcov19 / covidX

covidX. Rapid Pandemic Task Mgmt Portal
GNU General Public License v3.0
7 stars 17 forks source link

added custom fields to user model #69

Open ElijahAhianyo opened 3 years ago

ElijahAhianyo commented 3 years ago


This change is Reviewable

gitpod-io[bot] commented 3 years ago

codecakes commented 3 years ago

@ElijahAhianyo if working on JIRA tasks, use smart commits

codecakes commented 3 years ago

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

milan-tom commented 3 years ago

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.

ghost commented 3 years ago

Congratulations :tada:. DeepCode analyzed your code in 2.47 seconds and we found no issues. Enjoy a moment of no bugs :sunny:.

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

codecakes commented 3 years ago

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 :


[DEFAULT]
USERNAME = 
PASSWORD = 
EMAIL = 
DB_USERNAME = 
DB_PASSWORD = 

[verified]
User =
Password

[unverified]
similarly here = 

import six import configparser

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

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

milan-tom commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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())
milan-tom commented 3 years ago

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 commented 3 years ago

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;

lgtm-com[bot] commented 3 years ago

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

new alerts:

lgtm-com[bot] commented 3 years ago

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

new alerts:

lgtm-com[bot] commented 3 years ago

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

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging dba98d57534cfb9606c1116f3cd6366db0bb3ad2 into 323f872bb13a83c7caf3c92fce68aad5826a6f2b - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 24d49716ed4c9dc4d0b96a5b2f6910ddd5c1dc61 into 4e370bf96b4f1a5126bc0c056fb961ac8ec5b64c - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 6617ddcb98b160f41ce5d966475bfa5bad8dfa97 into 5899f0879b6deaa540666e771f022042d53bffff - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 1d0876c16f09a2f58f9afac840025e7444d0eff4 into 5899f0879b6deaa540666e771f022042d53bffff - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 99a9f6e284ad2b4ca699efc83b4e529a001698a0 into 5899f0879b6deaa540666e771f022042d53bffff - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 1fa07837a4ae9b68499ee6ff73e3836e639931f9 into 5899f0879b6deaa540666e771f022042d53bffff - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 279951f434aad7ce65007f769ae8d95f4131e5fb into 98e35ac22e04fcd6604544f0eedb04d1d38f6bd6 - view on LGTM.com

new alerts:

marcelloromani commented 3 years ago

@milan-tom > Formatting problems in some migrations ➔ Based upon my understanding, migrations shouldn't be edited manually

I have opened a bug about this. #84

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 56fbb521458f182dc62f62ead9d24291784577b0 into 98e35ac22e04fcd6604544f0eedb04d1d38f6bd6 - view on LGTM.com

new alerts:

codecakes commented 3 years ago

/gcbrun

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 09fd3f8e877a5cde8105dc7b17ff55c9c7c46d62 into 6169b743e820ec8669619e2d37794160fc3f35c8 - view on LGTM.com

new alerts:

codecakes commented 3 years ago

@ElijahAhianyo @milan-tom pls reformat with black: apps/auth_zero/migrations/0004_auto_20201227_1514.py and; apps/auth_zero/migrations/0005_user_email_verified.py

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 3da0a46587deef2778296e92066c66521296a1cd into 5991f6289f3d61055847ff9301da10b181c48f7c - view on LGTM.com

new alerts:

codecakes commented 3 years ago

reviewing this today

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 6a15f2b3b38575ee6c9d8ab6df60c3fbfeb0b3b8 into 5991f6289f3d61055847ff9301da10b181c48f7c - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 07c2cc410072499231356a7804c20127addb5bf7 into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging c5eec55ba4468972561ca38edb79a66d8fb09553 into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging f3db20ee823625e65e26b20063568c603ae9c735 into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 234e4ef3821051a4746a1f76aa8682b8e686061d into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging be6289df48595aec28596eddc58562f37cc9ac68 into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging b460878ab9cb6f7b2b5f67c34ad257b9cf5ea816 into 267f4ccadf2e69ffd0122fbd53790d8a8dee73d9 - view on LGTM.com

new alerts:

marcelloromani commented 3 years ago

Hi @ElijahAhianyo I tested the branch:

(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
codecakes commented 3 years ago

See https://github.com/Xcov19/covidX/pull/95 for reference what's changed

lgtm-com[bot] commented 3 years ago

This pull request introduces 2 alerts when merging fedd85126d63e4e24be03ecb71a0e6e34c40654f into 8d9f7f9dff444c19ed88f7da2d33052ee1ece9d0 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 3 years ago

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

new alerts:

marcelloromani commented 3 years ago

https://github.com/PyCQA/pydocstyle/issues/141

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

codecakes commented 3 years ago

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

codecakes commented 3 years ago

make sure apps.common.config absolute imports everywhere

codecakes commented 3 years ago

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 commented 3 years ago

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[bot] commented 3 years ago

gitpod-io[bot] commented 3 years ago

gitpod-io[bot] commented 3 years ago

gitpod-io[bot] commented 3 years ago

gitpod-io[bot] commented 3 years ago