Skip to content
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

Validate AWS IDs when users set their AWS IDs in the Edit Cloud Details information #2101

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

Chrystinne
Copy link
Contributor

This pull request validates AWS IDs to ensure they match the 12-digit numerical values expected for AWS account IDs when users set their AWS IDs in the Edit Cloud Details information.

@tompollard
Copy link
Member

Looks good to me, thanks Chrystinne. A couple of quick thoughts:

@tompollard
Copy link
Member

Following convention in validators.py, perhaps rename function to something like validate_aws_id?

Actually I see we have is_institutional_email, so perhaps something like is_aws_id fits after all.

@bemoody
Copy link
Collaborator

bemoody commented Oct 4, 2023

This logic should go in the form class if not the model class. It doesn't belong in views.py.

Probably the best thing would be to define it as a validator in the CloudInformation class. Try that and see if it works. If that doesn't work then define a clean_aws_id method in the CloudForm class.

@@ -1218,8 +1218,7 @@ class CloudInformation(models.Model):
on_delete=models.CASCADE)
gcp_email = models.OneToOneField('user.AssociatedEmail', related_name='gcp_email',
on_delete=models.SET_NULL, null=True)
aws_id = models.CharField(max_length=60, null=True, blank=True, default=None)

aws_id = models.CharField(max_length=60, null=True, blank=True, default=None, validators=[validators.validate_aws_id])
Copy link
Member

@tompollard tompollard Oct 4, 2023

Choose a reason for hiding this comment

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

I think you've lost the blank line before class Meta? It needs to be added back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tompollard I accidentally did. Thanks for catching that!

@Chrystinne
Copy link
Contributor Author

@bemoody Using a validator in the CloudInformation model class works. Thanks.

@@ -1,6 +1,7 @@
import logging
import os
import pdb
import re
Copy link
Member

Choose a reason for hiding this comment

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

this import is no longer needed.

@tompollard
Copy link
Member

Thanks @Chrystinne, looks good to me. My understanding is that validators won’t retroactively affect existing data, so there shouldn't be any problems with applying this on the live server.

Please could you squash the commits before we merge? Perhaps one commit for the addition of the validator, and one for the migration?

…mation model. The modification includes the addition of the 'validate_aws_id' validator to ensure AWS ID validity at the model level.
@tompollard
Copy link
Member

thanks!

@tompollard tompollard merged commit f71a607 into dev Oct 5, 2023
11 checks passed
@tompollard tompollard deleted the cf/validate_aws_ids branch October 5, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants