Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(apply tag dialog): fix type errors #1567

Merged
merged 6 commits into from
Feb 15, 2023
Merged

fix(apply tag dialog): fix type errors #1567

merged 6 commits into from
Feb 15, 2023

Conversation

ajohn25
Copy link
Contributor

@ajohn25 ajohn25 commented Feb 14, 2023

Description

This fixes type errors in the apply tag dialog component.

Motivation and Context

Closes #1565

How Has This Been Tested?

This has been tested locally.

Screenshots (if appropriate):

Documentation Changes

Checklist:

  • My change requires a change to the documentation.
  • I have included updates for the documentation accordingly.

@ajohn25 ajohn25 added this to the 6.1.0 milestone Feb 14, 2023
@ajohn25
Copy link
Contributor Author

ajohn25 commented Feb 14, 2023

Getting this up for review to unblock release of a working 6.1! There are some additional items to note for review here that I punted looking into in favor of fixing user facing behavior more quickly. If we have agreement on punting on these, we can turn them into followup issues:

  1. Are the unsafe React lifecycle methods no longer necessary with hook based state management or does this PR potentially introduce different unwanted component behavior? I have confirmed that saving + removing tags appears to work fine with this PR as written.
  2. What's going on under the hood with tag.createdAt to produce the GraphQL type error described in the last error block here?

@ajohn25 ajohn25 removed this from the 6.1.0 milestone Feb 14, 2023
@bchrobot
Copy link
Member

  1. Are the unsafe React lifecycle methods no longer necessary with hook based state management or does this PR potentially introduce different unwanted component behavior? I have confirmed that saving + removing tags appears to work fine with this PR as written.

First, to clarify, the unsafe lifecycle methods were getting deprecated for class-based components hooks or no hooks. There are more idiomatic ways of accomplishing the same behavior with modern class-based lifecycle methods. However, functional components using hooks provide the same functionality and are easier to reason about and make all class-based lifecycle methods, UNSAFE_ or not, no longer necessary.

In this case, it looks like the lifecycle methods were being used to reset the selected tags between openings of the dialog. It looks like you'll need to add that using useEffect to reset selected tags to props tags when open transitions from false to true.

  1. What's going on under the hood with tag.createdAt to produce the GraphQL type error described in the last error block here?

The graphql-date package expects all inputs to be JS date types and does not support ISO 8601 strings (tjmehta/graphql-date#2, code):

This is usually not an issue as Knex and pg-node take care of converting timestamptz columns to Dates. Tags are fetched as strings, however, in a fancy resolver move to bundle DB queries:

.select([
"campaign_contact_tag.*",
r.knex.raw("row_to_json(tag.*) as tag"),
r.knex.raw("row_to_json(public.user.*) as tagger")
]);

and then check whether this ^^ magic happened or whether we need to query as usual:

tag: async (cct: CampaignContactTagRecordPreloaded) =>
cct.tag ? cct.tag : r.reader("tag").where({ id: cct.tag_id }).first(),
tagger: async (cct: CampaignContactTagRecordPreloaded) =>
cct.tagger
? cct.tagger
: r.reader("user").where({ id: cct.tagger_id }).first(),

@ajohn25 ajohn25 requested a review from bchrobot February 15, 2023 01:53
Copy link
Member

@bchrobot bchrobot left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@hiemanshu hiemanshu left a comment

Choose a reason for hiding this comment

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

LGTM!

@ajohn25 ajohn25 merged commit c017a51 into main Feb 15, 2023
@ajohn25 ajohn25 deleted the fix-tag-editor branch February 15, 2023 20:20
@bchrobot bchrobot mentioned this pull request Feb 27, 2023
2 tasks
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.

Fix apply tag dialog behavior
3 participants