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

RFC: Update timestamps to reflect dev status changes #203

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Roystbeef
Copy link
Collaborator

@Roystbeef Roystbeef commented Jan 17, 2024

Summary

This PR adds logic to use the file's last modified time for a node when the node's dev status changes. This is to work around the fact that a node's lastModified time is not updated when its dev status changes in Figma.

In order to track this, I've added the following fields to associated_figma_design

  • devStatus
  • lastUpdated

The devStatus just tracks the last devStatus we read from the node so we can detect when it changes to use the file's last modified timestamp instead.

the lastUpdated track the last timestamp we sent to Jira. This is useful when the following series of events occurs:

  • A node's devStatus changes so we use the file's lastModified time
  • A subsequent file update has occurred but does not touch this node
    In this case, we want to use the timestamp we previously used on the node when only the devStatus changed

These fields start off as AtlassianDesignStatus.UNKNOWN and new Date(0).toISOString() which happen to be the same as when an associated design added using the backfill migration.

getLastUpdatedTimeForNode is where the bulk of the complexity lies:

const getLastUpdatedTimeForNode = ({
  newDevStatus,
  nodeLastModified,
  fileLastModified,
  prevDevStatus,
  prevLastUpdated,
}: {
  newDevStatus: AtlassianDesignStatus;
  nodeLastModified: string;
  fileLastModified: string;
  prevDevStatus: AtlassianDesignStatus;
  prevLastUpdated: string;
}): string => {
  if (newDevStatus !== prevDevStatus) {
    if (prevDevStatus === AtlassianDesignStatus.UNKNOWN) {
      // Use the existing lastModified time on the node if the cached devStatus
      // we have is UNKNOWN since we can't be certain that the file update
      // triggered the dev status to change
      return nodeLastModified;
    } else {
      // Use the lastModified time on the file if the devStatus changes.
      // Since any node changes cascade upwards, we can be certain that the
      // file.lastModified time > the node.lastModified time
      return fileLastModified;
    }
  } else {
    // If there has been no dev status change to the node, there are 2 cases we
    // need to consider:
    // - The node itself has changed without changing the dev status
    // - The node has not changed but we previously used the file last modified
    //   time when sending data to Jira
    // In either case, we just want to use whichever is the most recent timestamp
    if (
      getUpdateSequenceNumberFrom(prevLastUpdated) >
      getUpdateSequenceNumberFrom(nodeLastModified)
    ) {
      return prevLastUpdated;
    } else {
      return nodeLastModified;
    }
  }
};

Most of the time we want to use the last modified time we have on the node. The exceptions to this are:

  • We know that the devStatus has changed -> use the last modified time from the file
  • We previously used the file's last modified time and the node still hasn't changed -> use the stored last updated time

The devStatus and lastUpdated time we send to Jira are always written to the DB (for simplicity).

The one thing to call out is after we deploy this change, since we default to AtlassianDesignStatus.UNKNOWN, we'll miss any updates where the devstatus for a node has changed but the node itself hasn't. Subsequent changes to the devstatus for those nodes will, however be captured. IMO this is okay since we're running into this for every dev status change currently.

Test Plan

  • CI passes
  • Link a frame to an issue
  • Only update the ready for dev status of that frame
  • Go back to the issue
  • Assert that it shows up as Design Updated

@Roystbeef Roystbeef force-pushed the roy/dev-status-timestamp branch 2 times, most recently from 2099be4 to ae87602 Compare January 17, 2024 22:43
);
// Update the timestamp for all the designs that updated their dev status
await Promise.all([
await jiraService.submitDesigns(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Roystbeef I think I'm missing something here. Can you provide some detail about how this process works?

From what I can tell:

  • figmaService.getAvailableDesignsFromSameFile returns the node with the stored devStatusLastModified on line 47 above
  • Then here you send the design to Jira and update the devStatusLastModified at the same time

Wouldn't this send a stale value to Jira?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the description in the summary. I understand how this works now. My concern with this approach is it's a lot of complexity to workaround an issue that is better solved in Figmas API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My concern with this approach is it's a lot of complexity to workaround an issue that is better solved in Figmas API.

Yeah I agree that having Figma's API handle this is most definitely the better long-term solution. My understanding of the situation is the current lastModified value we currently expose for nodes just surfaces a value that previously was only used privately for a different set of use cases. There's a few approaches we could take for this API going forward (eg. building it with the REST API in mind so that devstatus changes (among others) are reflected in the timestamp).

Unfortunately, IMO this is the only feasible solution to getting this ready by GA and IMO feels better scoped since any tech debt we're introducing is only on the relevant integration :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good @Roystbeef and we're happy to defer to you as maintainer of the app however please consider this issue was not raised by customers but only found by us while testing. You could defer until Figmas API can be changed, keeping this workaround in your back pocket in case you receive feedback from customers.

@stannnous
Copy link
Collaborator

If there's logic in Jira's ingestion to ignore any updates where from designs where the lastModified time is older than the one it already has, we can remove devStatusLastModified and simplify the logic here.

Jira handles out of order processing and will store entities with older lastModified (updateSequenceNumber in the Jira API) if it hasn't received that version before, or if it has received that version, Jira will ignore the update. While it should not cause a problem, it is unusual because you'd be sending updated data with an older version number.

@Roystbeef
Copy link
Collaborator Author

Jira handles out of order processing and will store entities with older lastModified (updateSequenceNumber in the Jira API) if it hasn't received that version before, or if it has received that version, Jira will ignore the update.

hmm, this sounds like we have quite a bit of leeway then. In theory, we wouldn't need to keep track of the devStatusLastModified value, but for ease of debugging/correctness, I think our service should try to honor the contract of only having lastUpdated go up (also I already wrote the code for it haha)

While it should not cause a problem, it is unusual because you'd be sending updated data with an older version number.

There should be no difference in data when this occurs. Since neither the node nor its dev status will have been modified in this case.

@Roystbeef Roystbeef force-pushed the roy/dev-status-timestamp branch 3 times, most recently from 589d62b to ec33e57 Compare January 25, 2024 21:46
@Roystbeef Roystbeef changed the base branch from main to roy/webhook-route-duration January 25, 2024 21:46
@Roystbeef Roystbeef marked this pull request as ready for review January 26, 2024 01:19
Base automatically changed from roy/webhook-route-duration to main January 30, 2024 19:59
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.

2 participants