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

Don't make unnecessary history changes when updating RS inside agendapoints #700

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

lagartoverde
Copy link
Contributor

@lagartoverde lagartoverde commented Aug 16, 2024

Overview

When checking for RS updates we were making unnecesary changes and adding them to the history, which resulted in the editor thinking there were changes when there were not. This interfiered with the saving action.
I limited these changes, and I added a way of updating properties without adding them to the history

connected issues and PRs:

GN-4961
lblod/ember-rdfa-editor#1214

Setup

Link the ember-rdfa-editor with the branch of the PR I linked

How to test/reproduce

Go to the agendapoints, create a new agendapoint with a RS, save it and check that it doesn't say that it has unsaved changes anymore, modify the RS go back to the agendapoint and check that now you have a unsaved change to save

Challenges/uncertainties

Maybe in the future we shouldn't require user action in order to update the RS

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

With the if statement switched around, this seems to work as expected.
Some ideas/comments on another approach we could take:

  • When inserting a regulatory statement in a decision, directly populate the content attribute (without doing it using the did-insert hook
  • When saving a regulatory statement, we could update the all the linked documents in a headless way => this gives us some advantages:
    • No longer required to open agendapoint to update linked RS
    • We are sure AP's contain the latest version of a RS
      => Note: we should only do this with unpublished/unsigned AP's

@elpoelma
Copy link
Contributor

Ok, so essentially we could alter the code so it only updates the 'content' attribute when the content has actually changed.

@elpoelma elpoelma self-requested a review August 20, 2024 09:53
Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Works, this PR no longer needs the editor bump then right?

@lagartoverde lagartoverde merged commit 377255e into master Aug 21, 2024
3 checks passed
@lagartoverde lagartoverde deleted the GN-4961 branch August 21, 2024 08:18
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