Skip to content

Conversation

@phycodurus
Copy link
Member

@phycodurus phycodurus commented Aug 6, 2025

This enhances the ObservationCreateView by

  • Passes User instance to the Facility so it can use that to get data out of the Facility Profile. (Facility apps will know if they have a User profile or not)
  • Also puts the user in the kwargs of the Form class for the same reason.
  • Clean up instantiation itself so it only happens once per Request/Response cycle. This simplifies the changes above. What this means is that rather than calling self.get_facility_class()() all over the place, we call it once at the beginning (in dispatch()) and refer to self.facility_instance throughout the View.

These changes are prerequisites for

Also, attach the User instance to the Facility since the Facility
will need it to get the User-specfic credentials from the Facility
Profile.
@phycodurus phycodurus requested a review from jchate6 August 6, 2025 19:15
@phycodurus phycodurus moved this to Needs Review in TOM Toolkit Aug 6, 2025
phycodurus and others added 3 commits August 6, 2025 12:20
Recent changes to instantiate the facility class only once per
request cycle in `ObservationCreateView.dispatch` introduced
issues with form creation, causing three tests to fail.

This commit refactors the `get_form` method to correctly instantiate
the form class directly with the necessary keyword arguments,
including the user. It also handles potential `TypeError` for
forms that do not accept a `user` argument, for backwards
compatibility.
@phycodurus
Copy link
Member Author

Ready for review -- Now with passing tests!!

@phycodurus phycodurus marked this pull request as ready for review August 11, 2025 23:48
Copy link
Contributor

@jchate6 jchate6 left a comment

Choose a reason for hiding this comment

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

minor things...

kwargs['user'] = self.request.user
return kwargs

def post(self, request, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care for this as a method for handling this kind of error.
This feels like something we should be handling at the base form level or the get_form() method rather than trying to catch the exception here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't a error handling change. This is where we attach the User instance to the form kwargs. We do that here b/c the View has the request and the request has the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, maybe my comment was confusing.

We shouldn't find ourselves in a situation where adding a user to the form_class throws a TypeError.

We shouldn't be checking for that in the form Post in the view. Obviously we need to add the specific user instance in the view, but we should do this check (and add the capability to accept a user if needed) in the base form or the get_form() method.

If you'd like to have a chat about this, let me know.

@phycodurus phycodurus requested a review from jchate6 August 20, 2025 22:05
kwargs['user'] = self.request.user
return kwargs

def post(self, request, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, maybe my comment was confusing.

We shouldn't find ourselves in a situation where adding a user to the form_class throws a TypeError.

We shouldn't be checking for that in the form Post in the view. Obviously we need to add the specific user instance in the view, but we should do this check (and add the capability to accept a user if needed) in the base form or the get_form() method.

If you'd like to have a chat about this, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

3 participants