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: duplicate fields to next row #6285

Merged
merged 13 commits into from
May 21, 2023
Merged

Conversation

foochifa
Copy link
Contributor

@foochifa foochifa commented May 8, 2023

Problem

Currently when we duplicate fields, it is duplicated to the end of form fields. This makes it difficult for users of large forms to manoeuvre or modify their forms, and causes a lot of unneeded drag and drops.

This is related to our efforts to optimise performance for large forms in #6045

Solution

Form fields are now duplicated to the index immediately after the original field.

During implementation we had 2 potential methods of approaching this problem.

  1. Follows the current implementation, whereby Backend's admin-form.service will push the new duplicated field to end of form fields array. Whilst Frontend will do the same in useDuplicateFormField. Therefore, an explicit sync that we have to maintain between the two. So instead of pushing, both Backend and Frontend will look for the field to be duplicated and find its index + 1. This is the cleanest method of implementing this feat.
  2. The alternative is to pass the insertion index from Frontend to the Backend at /duplicate endpoint as a req.body. This will allow an implicit sync between the Frontend and the Backend as the insertion index is passed between them. This can help in extensibility in the future too, if we have other features that requires duplicating to certain indexes.

Decided on option 1, as it is the cleanest approach and the Backend does not rely on client input

Breaking Changes

Before & After Screenshots

BEFORE:

Screen.Recording.2023-05-08.at.6.26.15.PM.mov

AFTER:

Screen.Recording.2023-05-08.at.6.29.07.PM.mov

Tests

In a storage mode form

  • Create a few new fields (>2).
  • Duplicate the top field, the new duplicated field should appear just below the original field. It should be auto-focused by the drawer.
  • Make some edits to the new duplicated field
  • Refresh the page. The orders and details of the fields should still be the same (to ensure BE and FE queryclient are equivalent)
  • Duplicate the last field, to make sure no edge issues.
  • Refresh the page.
  • Go to form preview, the fields in preview mode should be as per admin form view
  • Open the form, the fields in public form should be as per admin form view

Email mode form

  • do the same for an email mode form

@foochifa foochifa marked this pull request as ready for review May 9, 2023 02:59
@foochifa foochifa temporarily deployed to staging-alt May 9, 2023 03:06 — with GitHub Actions Inactive
@foochifa foochifa requested review from justynoh and LinHuiqing May 10, 2023 08:41
Copy link
Contributor

@LinHuiqing LinHuiqing left a comment

Choose a reason for hiding this comment

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

Tested! But this PR is not backwards compatible! In the case where a new FE hits a old BE, the fields will be duplicated at the next index on FE but BE will save it at the end of the array of form fields. Conversely, if old FE hits new BE, FE will show that its added as the last field but BE will add it to the next one. This could be an issue with the volume of traffic we get, not sure if we have metrics as to how often admins duplicate fields?

@foochifa
Copy link
Contributor Author

Oh boy you are right... Thats the problem when FE and BE assumes each other knows, which is the problem of implementation 1

Not sure how we can circumvent this though? From speaking to Kenneth and Stacey, qualitatively, seems like duplicate is quite widely used.

Then perhaps it might be best to only push this change at night or early mornign? 😭

@LinHuiqing
Copy link
Contributor

Tested! But this PR is not backwards compatible! In the case where a new FE hits a old BE, the fields will be duplicated at the next index on FE but BE will save it at the end of the array of form fields. Conversely, if old FE hits new BE, FE will show that its added as the last field but BE will add it to the next one. This could be an issue with the volume of traffic we get, not sure if we have metrics as to how often admins duplicate fields?

Okay, checked and it could be up to ~50 in 5min. Could be substantial 🤔

@foochifa
Copy link
Contributor Author

Thanks @LinHuiqing, yeah I think we shouldn't release this during work hours... Will plan with the on-call person next week to release after hours!

@LinHuiqing
Copy link
Contributor

LinHuiqing commented May 11, 2023

Oh boy you are right... Thats the problem when FE and BE assumes each other knows, which is the problem of implementation 1

Not sure how we can circumvent this though? From speaking to Kenneth and Stacey, qualitatively, seems like duplicate is quite widely used.

Then perhaps it might be best to only push this change at night or early mornign? 😭

Yea actually I think there's no easy way to circumvent this that won't take too long :') If we really wanna do it safely, can break it up into multiple releases:

  1. Set up endpoint on BE for new duplicate endpoint
  2. Update FE to duplicated on next index and point to new BE endpoint when duplicating
  3. Deprecate / remove old duplicate BE endpoint

edit: points 1 + 2 can be done in the same release, then 3 in the next release

src/app/modules/form/admin-form/admin-form.service.ts Outdated Show resolved Hide resolved
return null
}

const index = formFields.findIndex((f) => fieldId === String(f._id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the String() wrapper necessary here when it wasn't used in useDuplicateFormField.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats because the f._id here is a Mongoose ObjectId whilst the one in useDuplicateFormField is a string hahaha

And the reason why I had to wrap it in string instead of using equals comparator is because for some reason typescript or our current mongoose type does not recognise ObjectId.equals which is very weird 😩

@wanlingt wanlingt merged commit dc72960 into develop May 21, 2023
@wanlingt wanlingt deleted the feat/duplicate-to-next-row branch May 21, 2023 13:23
@wanlingt wanlingt mentioned this pull request May 21, 2023
10 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.

4 participants