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

Refactor user actions to link to filing and submission #563

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

michaeljwood
Copy link
Contributor

Refactor user actions to link to filing and submission directly.

Remove direct links from FilingDAO and SubmissionDAO. Update DTOs to make existing links to user_action entities be derived. Update API accordingly.

closes #551

Need to do some more testing before calling this good (especially with migrating some of the data, as I didn't have any existing data points for signing for example). I also had a couple TODOs/questions to deal with.

The biggest questions I had were in the sign_filing method. I left the separate commit to create a sign action so that the DB timestamp could be used.

  1. Would that be better as a local timestamp so the action and the filing could be updated all at once (so it is pass/fail)?
  2. How can we guarantee that the email gets sent and the filing status is updated atomically (or make sure it can be recovered)?
  3. I was also a little concerned about the API not requiring the user to specify which submission they are signing. Presumably there are legal implications to signing a submission, so it seems to me that the API should require that to be specified rather than assume the latest in the DB is the one being signed, otherwise it could be susceptible to a race condition. Is this a legitimate concern, or am I being overly pessimistic?
  4. Lastly I was thinking about moving some entity creation into the repo instead of being in the API handler, but I wasn't sure if there were reasons to keep them there that I was overlooking.

…ubmission directly.

Remove direct links from FilingDAO and SubmissionDAO.  Update DTOs to make existing links to user_action entities be derived.  Update API accordingly.

closes cfpb#551
@computed_field
@property
def signatures(self) -> List[UserActionDTO]:
return [action for action in self.user_actions if action.action_type == UserActionType.SIGN]
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to make sure to maintain ordering so that the list is time ordered descending.

SIGN = "SIGN"
CREATE = "CREATE"
class UserActionType(StrEnum):
SUBMIT = ("SUBMIT",)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of using tuples here instead of straight strings? Seems unnecessary (nor do I see this as being a preferred approach)

Copy link
Contributor Author

@michaeljwood michaeljwood Jan 30, 2025

Choose a reason for hiding this comment

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

That was a formatting mistake. I had added commas (bad habits, whoops), and black automatically added the parentheses. Unfortunately I missed that when committing, but I'll fix it.

I changed it to StrEnum though, as I believe that is now the recommended approach as to my understanding the handling of (str, Enum) was changed in a minor Python version, and this allows me to use it in an f-string and have it replaced with the string value.

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.

Refactor user actions
2 participants