Implement support for a queried group check as a prerequisite for soc…

[Plushtoast]

Ok here is a revised pull request for the group check feature.

I extracted the whole group check logic into its own class, so it is easier to review.

I mainly removed:

• additional entry points e.g. journal enricher and skill picker instant check application (will pr that later if this is accepted
• minor things

What is still there that is not api is the rendering of the corresponding chat card. I felt like I need that for better testing.
In the corresponding ticket you proposed to keep an dialog open which is updating once results come in.
Instead i chose a chat card which is fire and forget (dialogs not in the way) and has a persistent storage for the data.
It seems to be also a more common pattern among systems to control this via chat buttons

Currently the request is still the same like before from within the StandardCheckDialog.

On Request it creates this in the chat:

All actor owners receive a roll request dialog.
Actors without an owner have to be rolled by the gm

After each roll the card is updated

The buttons next to the actor provide these functions.

• Request roll again (e.g. due to disconnect)
• Roll on behalf (e.g. player was beamed up by an alien and alien does not know how to click roll)
• Skip actor (e.g. someone else failed so hard that there is no hope anymore)

If you still want me to remove the chat implementation, I can do that as well. But i think from here on the PR is less confusing.

Fixes #97

[aaclayton]

I still need to finish reviewing the group-check.mjs file, but I have to go AFK now for a couple hours so I'm posting the comments that I have so far and I'll finish the review later.

[aaclayton]

Maybe this should become a GroupCheck instance method somehow? Is this.roll a GroupCheck instance in this case? If so it could be:

Suggested change

	static async #requestSubmit(_event, _target) {
	static async #requestSubmit(_event, _target) {
	await this.roll.requestSubmit();
	await this.close();
	}

[aaclayton]

Would this be StandardCheckData?

[aaclayton]

I wonder if this type can be generalized as something like a StandardCheckResult. The data structure here looks like it could apply to regular checks also

[aaclayton]

1. I think we should do this with fewer rounds of iteration
2. We shouldn't necessarily rule out user.isSelf
3. We should prefer non-GM owners to GM-owners
4. Prefer explicit null to undefined

Suggestion:

const users = [];
for ( const user of game.users ) {
  if ( !user.active ) continue;
  if ( user.character === actor ) users.push({user, priority: 1});
  else if ( user.isGM ) users.push({user, priority: 3});
  else if ( actor.testUserPermission(user, "OWNER") ) users.push({user, priority: 2});
}
users.sort((a, b) => a.priority - b.priority);
return users[0] ?? null;

[aaclayton]

Could this be reworked as an instance method where we construct the GroupCheck first and then from the constructed instance we submit it?

[aaclayton]

Instead of preparing these booleans and doing logic in the template, it would be better to prepare some data for status icon in javascript and render that icon directly in the template.

[aaclayton]

Instead of lots of conditional logic throughout this method, I think we should use a switch statement on entry.status since the statuses are mutually exclusive. Within each case of the switch statement we can modify the template data accordingly.

[aaclayton]

We could consider whether the receiving client needs to be told the dc of the check, or whether that piece of data could be reserved for only the requesting client (and the persisted chat message) to know.

This means we might not need the handling client to prepare the result data structure, the result could be determined only by the requesting client. All that the handling client needs to provide back is the result of their roll.

[aaclayton]

Prefer constructing the HTML element natively rather than parsing html:

Suggested change

	const btn = foundry.utils.parseHTML(
	`<button type="button" class="icon frame-brown fa-solid ${icon}" data-tooltip="${game.i18n.localize(tooltipKey)}"></button>`
	);
	const btn = document.createElement("button");
	btn.type = "button";
	btn.className = `frame-brown icon fa-solid ${icon}`;
	btn.dataset.tooltip = game.i18n.localize(tooltipKey);

[aaclayton]

Not sure there is any default click behavior that needs to be prevented here, so we can probably just bind the handler directly:

Suggested change

	btn.addEventListener("click", async event => {
	event.preventDefault();
	await handler();
	});
	btn.addEventListener("click", handler);

[aaclayton]

Perhaps you can use makeButton here too

[aaclayton]

Suggested change

[aaclayton]

Suggested change

	await message.update({ content, whisper: [], [`flags.crucible.${FLAG_KEY}`]: flags });
	await message.update({content, whisper: [], [`flags.crucible.${FLAG_KEY}`]: flags});

[Plushtoast]

ok good. very thorough review and good suggestion. before i proceed:

do you want to have the interaction via a chatcard or should i bringt that back into the dialog.
the chat card stores the data in flags, which is basically extra data stored that might not be required anymore.
if that would be a dialog the data could be discarded once the dialog is complete and you just have a plain chat card

since i expect players to be slow in rolling for many reasons and potentially future skills could allow a reroll on a failed check or sth, i would think so that the chatcard is the better long term solution and has the better ux

but up to your decision

next:

• do you need any sort of group result. currently it just basically list indiviudal results. but it could additionally do (if you define rules): define a threshold (e.g. 2 players need a success, a crit counts as 2 successes, a botch counts as -1 success), define an opposing role (higher role wins, define a defender (group) and attacker (group) higher sum wins, or define rolls with intervals e.g. a task needs 5 successes each roll takes 1h ingame, if the group does not achieve all 5 successes in one roll -- repeat duration increases ... etc
• some sort of individual boons pre selected for single characters (instead of everyone the same)
• hidden DC
• ... ?

[aaclayton]

do you want to have the interaction via a chatcard or should i bringt that back into the dialog. the chat card stores the data in flags, which is basically extra data stored that might not be required anymore. if that would be a dialog the data could be discarded once the dialog is complete and you just have a plain chat card

I'm happy with the chat card proposal.

do you need any sort of group result. currently it just basically list indiviudal results. but it could additionally do (if you define rules): define a threshold (e.g. 2 players need a success, a crit counts as 2 successes, a botch counts as -1 success), define an opposing role (higher role wins, define a defender (group) and attacker (group) higher sum wins, or define rolls with intervals e.g. a task needs 5 successes each roll takes 1h ingame, if the group does not achieve all 5 successes in one roll -- repeat duration increases ... etc

Yes, but let's keep that out of scope for this PR. I'd like to address the feedback here and get it merged and then we can take a next step to determine the aggregate group success or failure.

some sort of individual boons pre selected for single characters (instead of everyone the same)

We will need to make sure this integrates in with the actor hooks system for skill checks, but this should happen automatically if you are careful to go through Actor#getSkillCheck(). Is this currently used?

hidden DC

Can be out of scope for this PR.