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.

Sign up for GitHub

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

Jump to bottom

Add changes detection to record editing #1367

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Add changes detection to record editing #1367

merged 1 commit into from
Oct 30, 2023

Conversation

kelanik8
Copy link
Contributor

@kelanik8 kelanik8 commented Aug 2, 2023

The following changes are implemented

TODO: Summary

Changes in the user interface:

TODO: Add screenshots, recordings or remove this section

Checklist when submitting a final (!draft) PR

  • Commits are tidied up, squashed if needed and follow guidelines in CONTRIBUTING.md
  • Code builds
  • All existing tests pass
  • All new critical code is covered by tests
  • PR is linked to the relevant issue(s)
  • Rebased with the target branch

@kelanik8 kelanik8 requested a review from katrinDY August 2, 2023 13:59
@kelanik8 kelanik8 self-assigned this Aug 2, 2023
@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from 8fbe512 to a4b5aa7 Compare August 2, 2023 14:04
@kelanik8 kelanik8 linked an issue Aug 17, 2023 that may be closed by this pull request
Add change detection for record editing #1360
Closed
Fajfa
Fajfa reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@Fajfa Fajfa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Regarding this, please test it how it works with modals.
If all is good, move to QC so it can be confirmed.

Also rebase with 2023.3.x asap

@Fajfa Fajfa force-pushed the 2023.3.x branch 2 times, most recently from 77f8765 to 7254628 Compare August 25, 2023 09:26
@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from a4b5aa7 to d292008 Compare August 28, 2023 13:40
@katrinDY
Copy link
Contributor

katrinDY commented Sep 20, 2023 •
edited
Loading

A couple of issues:

  • in RL click on the dropdown menu for a record -> edit -> don't make any changes -> go back and the unsaved message is shown => not longer an issue
  • in RL click on the dropdown menu for a record -> edit -> don't make any changes -> click on save -> see the toast message Could not update record: Cannot read properties of undefined (reading 'recordID')
    1
  • open a record in modal -> edit a field -> ctr + z the change -> close the modal -> the unsaved message is shown (tried this on text, select and number type of field; the issue should be present for the rest of the field types) => Happens only on CRM not to new ns BE removes HTML and prevents change detection to work - ticket to be added for this by Tolu
  • click on add btn in RL -> add value to a field -> go back -> the unsaved message is not shown => won't be fixed since we're removing change detection when a resource sys created
  • open record in a modal -> click on add -> add value to a field -> close the modal -> the unsaved message is not shown => won't be fixed since we're removing change detection when a resource sys created

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from d67d958 to 39b01d3 Compare September 25, 2023 11:36
Fajfa
Fajfa requested changes Sep 25, 2023
View reviewed changes
Copy link
Member

@Fajfa Fajfa left a comment

Choose a reason for hiding this comment

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

Would it make sense to keep using the .clone(), but adjusting it on the class
So we can always use clone, similar to how you do it with other change detection

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from 20e8c7b to 994ec6d Compare October 5, 2023 13:58
@kelanik8 kelanik8 changed the base branch from 2023.3.x to 2023.9.x October 19, 2023 09:56
@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch 2 times, most recently from bf5d284 to 63d4e0e Compare October 19, 2023 11:00
@katrinDY
Copy link
Contributor

katrinDY commented Oct 20, 2023 •
edited
Loading

One last thing and we should be good to go: issue is present only in CRM and Case Management ns
1

@katrinDY
Copy link
Contributor

The issue is still present

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from 2ebbec5 to 02f68e4 Compare October 24, 2023 15:19
@katrinDY
Copy link
Contributor

  • happens only in CRM and Case management
    1

katrinDY
katrinDY reviewed Oct 26, 2023
View reviewed changes
client/web/compose/src/views/Public/Pages/Records/View.vue Outdated
@@ -640,6 +640,7 @@ export default {
checkUnsavedChangesOnModal (bvEvent, modalId) {
if (modalId === 'record-modal' && !this.inCreating) {
const recordStateChange = this.compareRecordValues() ? window.confirm(this.$t('general:editor.unsavedChanges')) : true
console.log('recordStateChange', recordStateChange)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this? If not, please, remove

@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from 24799a1 to 54f046b Compare October 30, 2023 10:14
@kelanik8 kelanik8 force-pushed the 2023.3.x-feature-add-change-detection-record-editing branch from 54f046b to 79791dc Compare October 30, 2023 10:16
@kelanik8 kelanik8 merged commit 79791dc into 2023.9.x Oct 30, 2023
1 check passed
@kelanik8 kelanik8 deleted the 2023.3.x-feature-add-change-detection-record-editing branch October 30, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katrinDY katrinDY katrinDY left review comments

@Fajfa Fajfa Fajfa approved these changes

Assignees

@kelanik8 kelanik8

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add change detection for record editing
3 participants
@kelanik8 @katrinDY @Fajfa