Skip to content

Evaluator Invitation Queue#135

Merged
3mcd merged 13 commits into
mainfrom
em/evaluator-invites
Oct 16, 2023
Merged

Evaluator Invitation Queue#135
3mcd merged 13 commits into
mainfrom
em/evaluator-invites

Conversation

@3mcd
Copy link
Copy Markdown
Collaborator

@3mcd 3mcd commented Oct 12, 2023

Issue(s) Resolved

Resolves #109

Test Plan

Comment on lines 10 to 11
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about this change, since not everyone has a first and last name. Is there some reason we need to separate these and require both?

I would prefer if we store people's name in a single column and split it where needed. Or at least make the lastName column nullable, which is what ORCID does.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go with last name nullable for now. We have a lot of issues on V6 with names since contributors only have one name field. Dutch names are apparently a huge headache esp. with a single name field.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eric's right, last name is needed for citation splitting purposes. But can definitely be nullable. For context, see:

knowledgefutures/pubpub#1063
knowledgefutures/pubpub#784

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible I'm not completely understanding how this is used, but if the extra fields let an integration add additional variables at runtime, will we have any way to tell users what those variables are (on our imaginary documentation pages)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular case seems like it could instead be solved by allowing the emails endpoint to take a pubId and offer {{ pub.* }} fields in the template context if that's passed.

That said, I guess it's already unclear how users should find out about the email syntax, since it's defined by core, but used by integrations. I'm hesitant to just say that integrations are responsible for describing the variables available because I think we will probably want to use the same template code for sending emails within core.

Copy link
Copy Markdown
Collaborator Author

@3mcd 3mcd Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invite link is tricky because it's kinda complex. It's an HTML element, which Unjournal has already expressed hesitation about using, and composed of 5 separate tokens, which makes the structure of the link even harder to understand.

Having a shorthand for common integration-specific URLs seemed like a decent way of solving this problem. The extras can't do anything regular interpolation/tokens can't already do. So it's not really introducing new/arbitrary runtime behavior. Just a mechanism for small partials/shorthands. extras aren't registered or anything. They're aliases just used in that single request to sendEmail.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invite link is tricky because it's kinda complex. It's an HTML element, which Unjournal has already expressed hesitation about using, and composed of 5 separate tokens, which makes the structure of the link even harder to understand.

Yeah, this makes sense and I understand not wanting to make them responsible for constructing that link. I think that's an issue that will probably keep coming up with these token links. Maybe we can eventually make that easier in whatever editor we end up using for these templates.

Having a shorthand for common integration-specific URLs seemed like a decent way of solving this problem. The extras can't do anything regular interpolation/tokens can't already do. So it's not really introducing new/arbitrary runtime behavior. Just a mechanism for small partials/shorthands. extras aren't registered or anything. They're aliases just used in that single request to sendEmail.

I agree that we should make all of this info available in the email templates, and we should make it simpler to construct those URLs. And I'm not asking you to change this right now. But I was thinking about David's comment today where he asked what variables were available in the email template. If we want to have some user-facing documentation about this template syntax and the available variables we have to either:

  • Put it in core, which means these extras can't be documented since they're defined in integration code.
  • Make the integrations responsible for displaying that documentation, since only they know what extras they've defined

I'm a little uneasy about the second option!

Comment thread core/pages/api/v0/[...ts-rest].ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to delete the invite pubs, but this seems like an endpoint we probably won't want to make available to anyone else, or we'll need to implement some kind of soft deletion.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, soft deletes are a must. I added a ticket here: https://github.com/pubpub/v7/issues/136

@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 12, 2023 21:04 — with Render Active
@3mcd 3mcd force-pushed the em/evaluator-invites branch from 946992f to 16b0d39 Compare October 16, 2023 13:50
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 13:50 — with Render Active
@isTravis isTravis temporarily deployed to em/evaluator-invites - integration-submissions PR #135 October 16, 2023 13:50 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - core PR #135 October 16, 2023 13:50 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - integration-evaluations PR #135 October 16, 2023 13:50 — with Render Destroyed
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 14:11 — with Render Active
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 14:21 — with Render Active
@3mcd 3mcd force-pushed the em/evaluator-invites branch from 0137cc5 to 3c0b408 Compare October 16, 2023 14:44
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 14:44 — with Render Active
@3mcd 3mcd force-pushed the em/evaluator-invites branch from 2ce2c6d to 8ab2a4a Compare October 16, 2023 18:32
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 18:32 — with Render Active
@isTravis isTravis temporarily deployed to em/evaluator-invites - core PR #135 October 16, 2023 18:32 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - integration-evaluations PR #135 October 16, 2023 18:32 — with Render Destroyed
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 18:52 — with Render Active
@3mcd 3mcd force-pushed the em/evaluator-invites branch from cc5d503 to fc9a02b Compare October 16, 2023 18:59
@isTravis isTravis deployed to em/evaluator-invites - flock PR #135 October 16, 2023 18:59 — with Render Active
@3mcd 3mcd force-pushed the em/evaluator-invites branch from fc9a02b to c051d27 Compare October 16, 2023 19:04
@isTravis isTravis temporarily deployed to em/evaluator-invites - flock PR #135 October 16, 2023 19:04 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - integration-submissions PR #135 October 16, 2023 19:04 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - integration-evaluations PR #135 October 16, 2023 19:04 — with Render Destroyed
@isTravis isTravis temporarily deployed to em/evaluator-invites - core PR #135 October 16, 2023 19:22 — with Render Destroyed
@3mcd 3mcd merged commit 96c43f1 into main Oct 16, 2023
@3mcd 3mcd deleted the em/evaluator-invites branch October 16, 2023 19:32
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.

Invite reviewers ordered list

5 participants