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

feat: check if changes have been made before publishing a new flow #574

Merged
merged 7 commits into from
Aug 13, 2021

Conversation

sarahscott
Copy link
Contributor

@sarahscott sarahscott commented Jul 22, 2021

The Problem

Currently, we flatten and publish flows every time a user clicks the Publish button. While this accomplishes our goal of allowing content to be manipulated and refined before it goes out to the public, it means we might be saving a lot of unnecessary data. Ideally, we can work toward something resembling version control.

Approaches

  1. Client-side diffing
    • Pros
      • Adds ability to disable Publish button when there aren't any new changes
      • Allows for more user feedback in the editor to indicate which changes have been made (i.e. similar to how we have a green border to track the steps of a flow in the preview browser, we could highlight changed nodes)
    • Cons
      • Potentially expensive for client browsers (only affects editors though; not general public)
      • Would need to make sure we avoid inconsistencies between our database and what is happening client-side
  2. Server-side diffing
    • Pros
      • Already implemented here 😄 (needs tests)
      • Easy
      • Heavier computation happens in the cloud instead of on our (& our colleagues') computers
      • Can diff server-side and send result back to client
    • Cons
      • Not as many interesting UX options for the editor
      • Moves us slightly away from the "programming language" model wherein PlanX is mostly contained client-side

Scope of this PR

  • Implement jsondiffpatch server-side
  • Add mocked JWT to jest tests
  • Tests

Future

  • Actually do something UI-side with altered nodes (e.g. highlight)

@netlify
Copy link

netlify bot commented Jul 22, 2021

✔️ Storybook

🔨 Explore the source changes: 1399125

🔍 Inspect the deploy log: https://app.netlify.com/sites/planx-storybook/deploys/6116ae760e321e0007f9751d

😎 Browse the preview: https://deploy-preview-574--planx-storybook.netlify.app

@github-actions
Copy link

github-actions bot commented Jul 22, 2021

@sarahscott sarahscott changed the title check if changes have been made before publishing a new flow feat: check if changes have been made before publishing a new flow Jul 22, 2021
@sarahscott sarahscott requested review from jessicamcinchak, gunar and johnrees and removed request for jessicamcinchak July 22, 2021 23:26
Copy link
Contributor

@gunar gunar left a comment

Choose a reason for hiding this comment

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

The question from today's call was: does this take into account portals? and if then, the published portals or the in-progress ones?

@sarahscott sarahscott marked this pull request as ready for review August 6, 2021 01:14
@sarahscott
Copy link
Contributor Author

Re: portals - I haven't changed anything about how the /publish endpoint deals with them -- they should be taken into account because the diff happens on "flattened" flows.

Copy link
Contributor

@johnrees johnrees 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 to me!! I ran it locally and everything worked great. I haven't tested it against of the huge flows yet but we can try that out in staging. Thanks!

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

this looks good!! appreciate the description with pros & cons you were thinking through and TESTS 🙌

@johnrees
Copy link
Contributor

@sarahscott sarahscott merged commit c26349a into main Aug 13, 2021
@sarahscott sarahscott deleted the sos/jsondiffpatch branch August 13, 2021 17:40
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.

4 participants