Liik 806 switch device type icons to uploaded files#651
Conversation
|
Commented next to azurite connection strings about them being public info. |
shundread
force-pushed
the
LIIK-806-switch-device-type-icons-to-uploaded-files
branch
2 times, most recently
from
7f56ad5 to
9bb1f9c
Compare
jforsman
left a comment
jforsman
left a comment
There was a problem hiding this comment.
Test coverage not enough, sonarqube should complain about it :D
traffic_control/management/commands/update_device_type_icon_field.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks to be ok. Did not find that many problems.
Tests are totally missing, but I guess that was known thing at this point.
Type annotation could be done for the new functions, I also forget to do that many times, we should do all of those at some point, I'll write a separate ticket on those.
You should also run translation string collections so the placeholders will be in place.
I guess the segration of icons and other azurestorage was needed for the icons overwrite setting? And what about having 2 different azure accounts? One for icons and the other for attachments?
|
|
||
|
|
||
| def city_furniture_device_type_icon_storage() -> Storage: | ||
| return storages["icons"] |
There was a problem hiding this comment.
This crashes if there is invalid configuration? What should be the default for this? And is it correct that the values is fetched from storages and not from settings?
There was a problem hiding this comment.
Q: This crashes if there is invalid configuration?
A: Yes, it does crash at launch if there is invalid configuration, is that bad?
Q: And is it correct that the values is fetched from storages and not from settings?
A: Yes, if we try to use settings.STORAGES["icons"] we get TypeError: FileField.storage must be a subclass/instance of django.core.files.storage.base.Storage
There was a problem hiding this comment.
Welll.. the thing is that this is not detected in deployment pipeline anywhere, so might cause some downtime if the configuration is missing, as the deployment goes ok but starting the app does not.
Ok by me if you just write a reminder ticket about this, so we do not forget it. It is pretty much a corner case.
traffic_control/management/commands/upload_static_svg_icons_to_blobstorage.py
Show resolved
Hide resolved
shundread
force-pushed
the
LIIK-806-switch-device-type-icons-to-uploaded-files
branch
4 times, most recently
from
bc02256 to
aead1f9
Compare
shundread
force-pushed
the
LIIK-806-switch-device-type-icons-to-uploaded-files
branch
3 times, most recently
from
1a50686 to
ddae2d5
Compare
…idget Refs: LIIK-806
We can use most of what was available for TrafficSign{Plan,Real}, except
we're dealing with a different field name (and slightly different model
structure), but template, javascript and CSS cane reused.
Refs: LIIK-806
The UI would crash when the user would add a new object using the inline + on the dropdown selector with icon preview. Now it simply quietly chooses not to show the icon when is not yet loaded. Refs: LIIK-806
Refs: LIIK-806
Other changes: * Remove deprecated version line on docker-compose.yml
Also refactor related city_furniture form/widget to use common implementation with traffic_control. Refs: LIIK-806
We don't know yet how allowing overwrite will affect other workflows yet, so we separate icon storage settings into from the default storage settings for now. Other changes on the related post_save signal handlers: * Make use of the file's storage info instead of default_storage * Don't make the PNG generation step exclusive to new files, or new PNG files won't be generated in case of SVG overwrite. Other notes: We have to handle in some manner the scenario where FooIcon gets created with with "foo.svg" and then later a user attempts to overwrite it with "bar.svg" The issue with such scenario is that with the current code it would leave "foo.svg" and its related "foo.png" files dangling in the storage, so one approach to fixing or preventing that needs to be done. * Prevent uploads with different name for an existing row, or * Forcibly rename uploads with different name for an existing row, or * Wipe old files as we're writing the new ones Refs: LIIK-806
…eleted Share implementation details of PNG generation signal handlers Refs: LIIK-806
* upload_static_svg_icons_to_blobstorage: Sends SVG icons to blobstorage, creates/updates associated TrafficControlDeviceTypeIcon objects. * update_device_type_icon_field: Updates TrafficcontrolDeviceType objects so that their icon_file field will point to the appropriate TrafficControlDeviceTypeIcon Refs: LIIK-806
shundread
force-pushed
the
LIIK-806-switch-device-type-icons-to-uploaded-files
branch
from
ddae2d5 to
e99d764
Compare
|
jforsman
left a comment
jforsman
left a comment
There was a problem hiding this comment.
Just remember to write a ticket or mention in maintenance guide about the mandatory setting.
|
|
||
|
|
||
| def city_furniture_device_type_icon_storage() -> Storage: | ||
| return storages["icons"] |
There was a problem hiding this comment.
Welll.. the thing is that this is not detected in deployment pipeline anywhere, so might cause some downtime if the configuration is missing, as the deployment goes ok but starting the app does not.
Ok by me if you just write a reminder ticket about this, so we do not forget it. It is pretty much a corner case.
shundread
deleted the
LIIK-806-switch-device-type-icons-to-uploaded-files
branch
2 participants
I'm sure there'll be many comments here. :)