-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add candidate field to RTEN model #190
Conversation
@@ -47,3 +49,4 @@ class PearsonRTENEvent(models.Model): | |||
content = models.JSONField() | |||
created_at = models.DateTimeField(auto_now_add=True) | |||
event_type = models.CharField(max_length=20, choices=EVENT_TYPE_CHOICES) | |||
candidate = models.ForeignKey(User, null=True, on_delete=models.SET_NULL) |
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.
Do you think we could use blank=True
, in order to don't be mandatory in forms or serializer.?
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 wanted to do this as mandatory as possible since the idea is that every record has a user however we don't know if the client will send the whole data in the example format and secondly the migration process complains about previous records when the null=True field is not present, that's why I added the null field and I don't think we should include the blank field
Returns: | ||
user (object or None): The user object associated with the anonymous user ID, or `None` if not found. | ||
""" | ||
anonymous_user_id = self.request.data.get("clientCandidateID", "").replace("NELC", "") |
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.
Ideally, we'd avoid modifying clientCandidateId(product requirement?) due to its impact on product functionality and user experience. Any changes would necessitate significant code updates related to anonymous IDs.
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.
125416e
to
196e88f
Compare
196e88f
to
c3f7bfa
Compare
Description
This adds a new User field to the RTEN model so that the record is associated with a User record in the database. This includes the required view modifications
Testing instructions
./manage.py lms migrate eox_nelp
ENABLE_CERTIFICATE_PUBLISHER
, this will enable the result notification pipeline and the external certificate publisher, its not necessary to test the external call so you can use an invalid mode, or if you want to test the external call setCERTIFICATE_PUBLISHER_VALID_MODES
with the desired modeBefore
After
Additional information
Include anything else that will help reviewers and consumers understand the change.
Checklist for Merge