Refactor forms #1005
Refactor forms #1005
Conversation
- Refactor what-buyers-will-see view - Refactor EditSupplierForm and EditContactInformationForm classes - Refactor EditSupplierForm and EditContactInformation form template - Combine EditSupplierForm and EditContactInformationForm
- Refactor EditRegisteredAddressForm and EditRegisteredCountryForm - Refactor registered_address template
- Refactor CompanyOrganisationSizeForm - Refactor edit_supplier_organisation_size template - Add TODO to edit_supplier_organisation_size view
- Refactor CompanyTradingStatusForm - Refactor edit_supplier_trading_status template - Refactor edit_supplier_trading_status view
lfdebrux
force-pushed
the
ldeb-refactor-forms
branch
from
5b05402
to
ecd6f65
Compare
| registered_address_form.address1.data = supplier['contact'].get('address1') | ||
| registered_address_form.city.data = supplier['contact'].get('city') | ||
| registered_address_form.postcode.data = supplier['contact'].get('postcode') | ||
| data_api_client.update_contact_information( |
There was a problem hiding this comment.
Sigh, we really don't have a way of updating both Supplier and ContactInformation together transactionally do we? I guess that's one reason we have so much FE validation for this data...
| form = CompanyTradingStatusForm() | ||
|
|
||
| prefill_data = {} | ||
| if request.method == "GET": |
There was a problem hiding this comment.
Why does this view only conditionally load the existing supplier data and the others load it for both GET and POST?
There was a problem hiding this comment.
I was worried about making the GET more expensive; the other views GET was already including the api requests.
I'm happy to take it out though, it is a bit premature optimisation-y
There was a problem hiding this comment.
I was actually looking at it the other way around - why don't we do it in all the others too? One advantage I can think of is a small amount of added clarity - it's more clear the prefill_data can't have any effect in the POST case if it never gets filled out in the first place.
Unless, I reasoned, the use of supplier-fetching here is being used as a guard to generate 404s if the supplier doesn't exist - but... that's ... not possible here? Guessing most of this app's views use a login_required decorator that requires role == "supplier"? (edit: just checked, yes it does)
Here's some refactoring I did of some of the forms in the supplier frontend.
The changes are part of what I was doing https://github.com/alphagov/digitalmarketplace-supplier-frontend/tree/ldeb-107-wtforms-dmfields-supplier-forms, but packaged nicer.
I want these changes to be in the supplier frontend now because I intend to reuse (some of) the code for the admin frontend, and it would be helpful to be able to point to the supplier frontend code in
masteras an exemplar.