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

KAS-4805 implement emptying interal review private comment feature #2266

Merged
merged 10 commits into from
Jan 7, 2025

Conversation

ValenberghsSven
Copy link
Contributor

@ValenberghsSven ValenberghsSven commented Dec 5, 2024

https://kanselarij.atlassian.net/browse/KAS-4805

Noticed that agendaitem.privateComment is still being filled in on PROD.
So in this code I added it's removal. That does mean saving agendaitems so I copied some the "approveAllAgendaitems" to show a loader and refreshing the route.
This is only needed untill we remove that property from agendaitem model in follow up ticket.

1 side tracking: I discovered that we used agenda.serial**N**umber instead of the correct agenda.serialnumber in some places.
Need to check if there are any consequences of that.

@@ -38,7 +38,7 @@ export default class NewslettersIndexController extends Controller {
const agendas = await meeting.agendas;
return agendas
?.slice()
.sort((a1, a2) => a1.serialNumber.localeCompare(a2.serialNumber))
.sort((a1, a2) => a1.serialnumber.localeCompare(a2.serialnumber))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is sidetracking, should not be capital N. No errors ever came of this? latestAgenda could have been incorrect in this view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally found where this has an effect.
right here (screenshot on PROD)
image

If there is more then 1 agenda version > no modified.
Only modified when there is only agenda A.
This should be fixed after merging this.

Copy link
Contributor

@tdn tdn left a comment

Choose a reason for hiding this comment

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

One small remark left. Works well otherwise in happy flow.

app/components/agenda/agenda-header/agenda-actions.js Outdated Show resolved Hide resolved
// TODO KAS-4886 remove when property is removed from model
if (!isEmpty(agendaitem.privateComment)) {
agendaitem.privateComment = '';
return await agendaitem.save();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have some unintended effects on approved agendas that have a next version. Will need to implement an extra check here, or wait until https://kanselarij.atlassian.net/browse/KAS-4886 is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this by passing reverseSortedAgendas from args. We can use that to determine the latest agenda without awaiting. Improved check to also only allow the action on the latestAgenda.

@ValenberghsSven
Copy link
Contributor Author

ValenberghsSven commented Dec 12, 2024

In our call to the backend we still passed the privateComment so both internal review and agendaitem model was set.
I put that on null in this ticket so we stop doing that for agendaitem, which would mean less or next to no agendaitem.save on new data.
Cleanup in follow up ticket to remove the code, also in agenda-submission

@ValenberghsSven
Copy link
Contributor Author

Pair reviewed with Tom

@ValenberghsSven ValenberghsSven merged commit ed0911d into development Jan 7, 2025
0 of 2 checks passed
@ValenberghsSven ValenberghsSven deleted the feature/KAS-4805-empty-internal-review branch January 7, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants