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/46 producttype nl datamodel #2

Merged
merged 8 commits into from
Dec 11, 2024

Conversation

Floris272
Copy link
Contributor

@Floris272 Floris272 commented Nov 28, 2024

Add the productypen app.

Changes from prototype

  • model and fields translated to Dutch
  • removed icons and images
  • removed Field & Condition model
  • switched from tinymce to markdownx for now.
  • removed open-forms-client and openformsslugField from producttype
  • removed related_producttypes

@Floris272 Floris272 requested a review from Coperh December 3, 2024 10:48
@Floris272 Floris272 self-assigned this Dec 3, 2024
@Coperh
Copy link

Coperh commented Dec 4, 2024

Something to consider: the default_admin_index.json is wrong, uses old untranslated name

If I remember correctly, there is a command to do this. Can run when everything is added.

Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

Looked through it a bit and nothing seems wrong sof far, Django or python wise.

I am not familiar with the model structure, so I am assuming they are correct.

Will continue on Friday

Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

Important things:

  • Productype admin has a typo breaking the change list
  • Vraag is missing all help_texts.
  • Price does not allow two prices with different product types to have the same start date

Moderately important stuff:

  • Markdownx fields are vague on what they actually support
  • UPN importer logging could be clearer
  • Aspects of UPN importer not being tested
  • Request mock or VCR for importing from URL (to see if it gives the data you want)
  • Onderwerpen model tests (for published conditions)

Not really import (QOL)

  • Search fields and filters in the admin might be useful
  • UPN importer & parser refactoring
    • Transaction atomic
    • Explicitly require url or local file and do not infer
    • Deal with or log duplicate values
    • Remove Temp file

src/open_producten/producttypen/admin/link.py Show resolved Hide resolved
src/open_producten/producttypen/admin/producttype.py Outdated Show resolved Hide resolved
src/open_producten/producttypen/admin/link.py Show resolved Hide resolved
src/open_producten/producttypen/admin/onderwerp.py Outdated Show resolved Hide resolved
src/open_producten/producttypen/tests/test_load_upn.py Outdated Show resolved Hide resolved
src/open_producten/producttypen/tests/test_load_upn.py Outdated Show resolved Hide resolved
src/open_producten/producttypen/tests/test_load_upn.py Outdated Show resolved Hide resolved
@Floris272 Floris272 requested a review from Coperh December 9, 2024 16:47
Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

The major issues have been improved and a lot of the QOL stuff too.

A few bare except statements instead of specific errors, but its quite minor and only in the command.

Comment on lines +41 to +44
def has_change_permission(self, request, obj=None):
if obj and obj.actief_vanaf < datetime.today().date():
return False
return super().has_change_permission(request, obj)
Copy link

Choose a reason for hiding this comment

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

I still feel that this will come up as an issue in the future, but tomorrows problems are for tomorrow's me.

And by me I mean you.


self.stdout.write(f"Done ({created_count} objects).")
response = requests.get(url)
if response.status_code != 200:
Copy link

Choose a reason for hiding this comment

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

Not sure if you intended it, but this will not catch 400 or 500 codes, only different 200 and 300 codes

Copy link

Choose a reason for hiding this comment

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

requests throws exceptions for those codes


UniformProductNaamModel.objects.exclude(id__in=upn_updated_list).update(
removed_count = UniformProductNaamModel.objects.exclude(
Copy link

Choose a reason for hiding this comment

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

This gets the count but IDK how. Should it not return a queryset? or is str(querset) always the count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.update() returns the number of affected rows.
docs

src/open_producten/producttypen/tests/test_load_upn.py Outdated Show resolved Hide resolved
@Floris272 Floris272 merged commit 6c11dc7 into master Dec 11, 2024
13 checks passed
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.

2 participants