Skip to content

Conversation

@Dirklectisch
Copy link
Contributor

@Dirklectisch Dirklectisch commented Oct 16, 2025

This PR makes it possible to add PractitionerRoles through the MCSD Admin application.

This form included options that spanned multiple fields and I've therefore introduced some code that handles field names in this format.

telecom[0][Value]

The backend parses this format using some helper functions in the new formdata module.

Needed to add some more logic to the frontend to handle these more complex option pairs. Alternative approach would be to render that part in the backend but I figured it would still require some JS in that case to say count the amount of current options filled in.

Additionally this has some cool HTMX stuff around error handling. The server will now figure out based on the headers if the error needs be rendered inline or as a full page and render the appropriate HTML.

@Dirklectisch Dirklectisch marked this pull request as ready for review October 22, 2025 15:25
@Dirklectisch Dirklectisch requested a review from a team as a code owner October 22, 2025 15:25
@gasperr
Copy link
Contributor

gasperr commented Oct 23, 2025

I scrolled over and have no comments. It looks good to me.

@Dirklectisch Dirklectisch merged commit c79e48b into main Oct 23, 2025
1 check passed
@JorisHeadease
Copy link
Contributor

Was starting a review earlier, but saw it already got accepted. Primarily was wondering about the usage of the backend parser, it has some limitations, and requires custom parsing logic. I think we can remove quite some code, and stay closer to the html spec by using repeated field names?

Say we have this HTML:

<select name="telecom-system">phone</select>
<input name="telecom-value" value="123">

<select name="telecom-system">email</select>
<input name="telecom-value" value="[email protected]">

When submitted, r.PostForm[<NAME>] gives you all values ([]string):

r.PostForm["telecom-system"] = []string{"phone", "email"}
r.PostForm["telecom-value"] = []string{"123", "[email protected]"}

Then just zip the arrays together:

systems := r.PostForm["telecom-system"]
values := r.PostForm["telecom-value"]

for i := range systems {
    contactPoint := fhir.ContactPoint{
        System: contactPointSystemFrom(systems[i]),
        Value:  &values[i],
    }
    role.Telecom = append(role.Telecom, contactPoint)
}

@Dirklectisch
Copy link
Contributor Author

Dirklectisch commented Oct 23, 2025

@JorisHeadease I thought about that approach and it would indeed result in simpler code. It would require the form data to always arrive in the same order or the zipping together would combine the values in the wrong way. If someone leaves a field empty that could also be an error prone situation.

Do you feel we can trust the ordering here to be deterministic?

@JorisHeadease
Copy link
Contributor

I would actually argue that the form order is safer via HTML (its submission order is guaranteed by the HTML spec) than in Go, the current code uses map[index]map[key]value{}, but Go maps do not guarantee order.

The empty field is a valid concern, but the inputs are required, so that would rule out the form being submitted. We could also add a defensive check in the background to verify the count equals or so.

@Dirklectisch
Copy link
Contributor Author

I would actually argue that the form order is safer via HTML (its submission order is guaranteed by the HTML spec) than in Go, the current code uses map[index]map[key]value{}, but Go maps do not guarantee order.

I didn't realise the ordering was guaranteed by the spec and yes while hash maps don't have deterministic ordering I don't think that matters for the code I wrote. In any case it's a good suggestion I might have a good at implementing it If I have a quiet moment. Thanks!

@JorisHeadease
Copy link
Contributor

Sounds good, cheers @Dirklectisch!

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