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

Feature/register necessary fields #248

Merged
merged 6 commits into from
Jun 8, 2022

Conversation

annashamray
Copy link
Contributor

@annashamray annashamray commented May 25, 2022

@annashamray annashamray added the wip Work in progress label May 25, 2022
@annashamray annashamray force-pushed the feature/register-necessary-fields branch from c31a645 to d37d72b Compare May 25, 2022 15:06
@annashamray annashamray removed the wip Work in progress label May 27, 2022
@annashamray annashamray requested review from JostCrow and vaszig May 27, 2022 16:05
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

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

I think @JostCrow should have a look as well. have only some remarks-questions after reading and testing your code.

@@ -59,6 +59,30 @@ class Meta:
)


class NecessaryUserForm(forms.ModelForm):
invite = forms.ModelChoiceField(
queryset=Invite.objects.all(),
Copy link
Member

Choose a reason for hiding this comment

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

this will be evaluatied upon loading open-inwoner. Should be populated in the init

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thougth it would be ok, since django querysets are lazy and should not be executed before running. Locally I tried loading OIP main page (with and without logging as a user) and there were no DB queries to Invite table.
Or did I misunderstand your comment?

Copy link
Member

Choose a reason for hiding this comment

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

I think you indeed misunderstood. Doing this means afaik executing the queryset when starting/reloading runserver, the queryset won't be executed when instantiating the form. In my experience you need to make your queryset a callable, in that case it should work as expected.

However it could be that my knowledge is rusty, I noticed that they have an example in the Django documentation which does the same: https://docs.djangoproject.com/en/4.0/topics/forms/modelforms/#a-full-example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also use this pattern for the Plan form. Should I leave it as it is or change it everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Lets leave it as is. I've tested adding a plantemplate on the testenvironment and this worked so I was incorrect, sorry!

@annashamray annashamray force-pushed the feature/register-necessary-fields branch from 63de078 to 985ce46 Compare June 1, 2022 16:35
@annashamray annashamray requested a review from alextreme June 2, 2022 08:14
@alextreme alextreme merged commit f04f510 into develop Jun 8, 2022
@alextreme alextreme deleted the feature/register-necessary-fields branch June 8, 2022 17:08
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.

3 participants