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

GN-4159 - Feature/create revoke signature button #442

Merged
merged 55 commits into from
Jun 8, 2023

Conversation

lagartoverde
Copy link
Contributor

No description provided.

@lagartoverde lagartoverde marked this pull request as draft March 22, 2023 13:42
@lagartoverde lagartoverde self-assigned this Mar 22, 2023
@lagartoverde lagartoverde changed the base branch from master to feature/update-publication March 22, 2023 14:02
@lagartoverde
Copy link
Contributor Author

Base automatically changed from feature/update-publication to master March 23, 2023 21:27
Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

why all the model.get calls? that method is deprecated in ED 4.x, and even before it was not "supposed" to be used.

not tested, since this PR broke the signing button, but that could be something with my setup

app/components/signatures/timeline-step.js Outdated Show resolved Hide resolved
app/components/publishing-log-hash.js Outdated Show resolved Hide resolved
app/models/agenda.js Show resolved Hide resolved
app/models/versioned-agenda.js Outdated Show resolved Hide resolved
@elpoelma
Copy link
Contributor

elpoelma commented Jun 3, 2023

reported by niels in rocket chat:

* toggle of people present at meeting is broken

* people present at voting don't load

* sign agenda as laarne writer, delete signature
  log out
  log in as laarne reader
  sign agenda
  -> error

first two probably related to ember data still EDIT: can not reproduce the first two EDIT: also can't repro the last but awaiting more testing

I reproduced the second issue (people present at voting don't load) when upgrading to ember-data 4.8, but I fixed it in that PR. I could not reproduce the error on ember data 3.28.13. The main issue was that the relation of people present at the meeting was not correctly awaited.

@elpoelma
Copy link
Contributor

elpoelma commented Jun 5, 2023

Signing the notulen should not prevent the use from switching whether a certain decision is published (included) in the notulen or not. It looks like this is no longer possible on this branch. At least it requires a manual refresh by the user.

Further more it looks like the signing takes the settings into account, where it was previously specified the whole document (all full decisions) should be signed at all times.

on master after signing: Screenshot from 2023-06-01 22-04-44

on this branch after signing: Screenshot from 2023-06-01 22-06-21

I was also able to reproduce the same issue, it only shows the toggle again after a full manual refresh. Will check if it occurs on master too.
EDIT: I wasn't able to reproduce it on master.

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.

Seems to work well overall. The new publication actions page works as expected.
I could also reproduce some issues @nvdk noticed, including:

  • the make public toggles no longer show up after signing notulen, they only show up after a manual browser refresh

I had no issues with removing a signature, signing out, signing in and adding a signature again.

app/components/publishing-log-document-name.js Outdated Show resolved Hide resolved
@elpoelma
Copy link
Contributor

elpoelma commented Jun 5, 2023

Seems to work well overall. The new publication actions page works as expected. I could also reproduce some issues @nvdk noticed, including:

* the `make public` toggles no longer show up after signing notulen, they only show up after a manual browser refresh

I had no issues with removing a signature, signing out, signing in and adding a signature again.

The issue with the toggles not being displayed is because the Signatures::Notulen::BehandelingenList component in the notulen route is not being rendered after the signature addition/removal. This is due to the in-element helper not fully working as expected.

{{#in-element this.containerElement}}
      <Signatures::Notulen::BehandelingenList
        @notulen={{this.notulen}}
        @prepublishedBehandelingen={{this.treatments}}
        @publicBehandelingUris={{this.publicBehandelingUris}}
        @toggle={{this.togglePublicationStatus}}
        @toggleAll={{this.toggleAllPublicationStatus}}
        @allBehandelingPublic={{this.allBehandelingPublic}}
      />
{{/in-element}}

this.containerElement is correctly updated after the signature addition/removal, but the in-element helper does not pick this up and thus no longer renders the Signatures::Notulen::BehandelingenList component.

@elpoelma
Copy link
Contributor

elpoelma commented Jun 5, 2023

Seems to work well overall. The new publication actions page works as expected. I could also reproduce some issues @nvdk noticed, including:

* the `make public` toggles no longer show up after signing notulen, they only show up after a manual browser refresh

I had no issues with removing a signature, signing out, signing in and adding a signature again.

The issue with the toggles not being displayed is because the Signatures::Notulen::BehandelingenList component in the notulen route is not being rendered after the signature addition/removal. This is due to the in-element helper not fully working as expected.

{{#in-element this.containerElement}}
      <Signatures::Notulen::BehandelingenList
        @notulen={{this.notulen}}
        @prepublishedBehandelingen={{this.treatments}}
        @publicBehandelingUris={{this.publicBehandelingUris}}
        @toggle={{this.togglePublicationStatus}}
        @toggleAll={{this.toggleAllPublicationStatus}}
        @allBehandelingPublic={{this.allBehandelingPublic}}
      />
{{/in-element}}

this.containerElement is correctly updated after the signature addition/removal, but the in-element helper does not pick this up and thus no longer renders the Signatures::Notulen::BehandelingenList component.

The issues is that the containerElement property is not a tracked getter, so the in-element does not pick it up. Ideally, something like this:

get containerElement() {
    if (this.loadNotulen.isIdle) {
      return document.getElementById(this.behandelingContainerId);
    } else {
      return null;
    }
  }

should work. Not super clean, but it ensures that containerElement is tracked each time the loading state changes.

@nvdk
Copy link
Member

nvdk commented Jun 5, 2023

@elpoelma wouldn't just making behandelingContainerId tracked work as well?

@elpoelma
Copy link
Contributor

elpoelma commented Jun 5, 2023

@elpoelma wouldn't just making behandelingContainerId tracked work as well?

The issue is that behandelingContainerId does not change.

@nvdk
Copy link
Member

nvdk commented Jun 5, 2023

ah right 😅

@abeforgit
Copy link
Member

So your suggestion works @elpoelma but I have no idea why since that whole block is already in an if-block that conditionally renders on the loadNotulen idle state

@abeforgit abeforgit requested review from nvdk, dkozickis and elpoelma and removed request for usrtim and abeforgit June 6, 2023 17:34
@abeforgit abeforgit changed the title Feature/create revoke signature button GN-4159: Feature/create revoke signature button Jun 6, 2023
@abeforgit abeforgit changed the title GN-4159: Feature/create revoke signature button GN-4159 - Feature/create revoke signature button Jun 6, 2023
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.

Seems to work well now, not encountering technical issues any longer. Just a note,
image
In the above page the margins of the back button are rather small, maybe we could increase it?

@abeforgit abeforgit requested a review from elpoelma June 7, 2023 15:54
@abeforgit
Copy link
Member

to save you some time, it looks like this now:
image

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.

Good to go now :)

@abeforgit abeforgit merged commit 6864014 into master Jun 8, 2023
@abeforgit abeforgit deleted the feature/create-revoke-signature-button branch June 8, 2023 14:57
@abeforgit abeforgit added the enhancement New feature or request label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants