-
Notifications
You must be signed in to change notification settings - Fork 540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Access Card management #3506
Conversation
blank=True, | ||
help_text=_('Optional notes') | ||
) | ||
user = models.ForeignKey( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add on_delete
attribute. I recommend you on_delete=models.SET_NULL
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any limits? Can every user have an unlimited number of cards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, User can have any number of cards.
src/ralph/access_cards/models.py
Outdated
RalphUser, | ||
null=True, | ||
blank=True, | ||
related_name='used_access_cards', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any usage of backward relation. Please consider +
if you don't need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
src/ralph/access_cards/models.py
Outdated
RalphUser, | ||
null=True, | ||
blank=True, | ||
related_name='owned_access_cards', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
blank=True, | ||
help_text=_('Optional notes') | ||
) | ||
user = models.ForeignKey( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any limits? Can every user have an unlimited number of cards?
) | ||
|
||
def __str__(self): | ||
return _('Access Card: {}').format(self.visual_number) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better override the default autocomplete queryset in order to filter out liquidated cards.
def get_autocomplete_queryset(cls):
return cls._default_manager.exclude(
status=AccessCardStatus.liquidated.id
)```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
'visual_number':'654321', | ||
'system_number':'F9876DSGV', | ||
'notes':'test note', | ||
'issue_date':'2020-01-02' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
region
is a required field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
class AccessCardFactory(DjangoModelFactory): | ||
visual_number = factory.Sequence(lambda n: '000000000000{}'.format(n)) | ||
system_number = factory.Sequence(lambda n: '000000000000{}'.format(n)) | ||
status = AccessCardStatus.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
region is a required field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
) | ||
self.assertEqual( | ||
response_data['owner']['username'], | ||
access_card.owner.username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check the region as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.