Features: Return publishing notes for the Form and Versions API

[sadiqkhoja]

Towards getodk/central#1834

What has been done to verify that this works as intended?

Added test, checked with frontend.

Why is this the best possible solution? Were any other approaches considered?

It is small and simple change to achieve the objective. I thought about introducing symbol/attribute in our Frame framework for restrictedReadonly field but that would have been a complicated change. We may consider that approach if there is proliferation of conditional fields in our APIs.

I added publishingNotes field for the /forms/:xmlFormId as well because we need to show notes for the "Published" Form page as well.

Is publishingNotes a right name for this field? We can call this just notes.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Once related frontend PRs are merged, QA team should test all Form and Form version functionality.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

Added.

Before submitting this PR, please make sure you have:

• run make test and confirmed all checks still pass, or witnessed Github completing all checks with success
• verified that any code from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

Is publishingNotes a right name for this field? We can call this just notes.

I like publishingNotes more than notes. 👍 Seems more descriptive. I'm not sure the "ing" is really needed, maybe could be the shorter publishNotes, but either seems good to me.

[matthew-white]

How about adding an example, I like the string from above:

example: Fixed validation rules for required fields

[matthew-white]

Suggested change

	description: The notes provided when this version was published (via the `X-Action-Notes` header). Only returned to users with `form.update` permission.
	description: The notes provided when the current Form version was published (via the `X-Action-Notes` header). Only returned to users with `form.update` permission.

[matthew-white]

I would either remove "(conditionally)" (it's explained below) or explain here what the condition is (maybe in a follow-up sentence). I think the sentence is slightly unclear as-is.

[matthew-white]

Could we require requiredVerbs to be an array? Since it's only called from canReadForm(), I feel like that'd be reasonable. I think there's enough going on in _checkActorPermission() that it'd be nice to simplify that aspect of it and remove this line. I know it's just one line though. 😅

[matthew-white]

Suggested change

	const _checkActorPermission = (authOrVerbs, requiredVerbs, form) => {
	const _checkActorVerbs = (authOrVerbs, requiredVerbs, form) => {

Just thinking that we don't use the term "permission" very much in the codebase.

[matthew-white]

What do you think about always passing _checkActorPermission() verbs, not auth? It'd be easy for canReadForm() to convert auth to verbs. I feel like _checkActorPermission() is an important function, but it's slightly on the complicated side right now. Sometimes it calls auth.canOrReject(), while other times it reimplements some of the logic from auth.can().

I'm thinking of something like:

const checkActorVerbs = (actorVerbs, requiredVerbs) => {
  const actorVerbsSet = new Set(actorVerbs);
  if (requiredVerbs.every(v => !actorVerbsSet.has(v))) {
    return reject(Problem.user.insufficientRights());
  }
  return true;
};

const canReadForm = async (authOrVerbs, form) => {
  const verbs = authOrVerbs instanceof Array
    ? authOrVerbs
    : (await auth.verbsOn(form));
  if (form.def?.publishedAt == null) {
    await checkActorVerbs(verbs, ['form.update']);
  } else if (form.state === 'closed') {
    await checkActorVerbs(verbs, ['form.read']);
  } else {
    await checkActorVerbs(verbs, ['open_form.read', 'form.read']);
  }
  return form;
};

I can definitely see the reasoning behind _checkActorPermission(). It prevents a second call to auth.can() when checking whether the user can read publishingNotes. I'm just wondering whether there's a way to make the auth logic crystal clear.

[matthew-white]

This describe() block doesn't check public links, while the describe() block above does. Personally, I feel like that's OK. Authz is driven by verbs, not actor type.

[matthew-white]

Love this scenarios file!