-
Notifications
You must be signed in to change notification settings - Fork 26
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
FIX: restore button to add user notes to post admin menu #85
Conversation
store, | ||
attrs.user_id, | ||
(count) => { | ||
userController.set("userNotesCount", count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not fully replicate the behaviour that was in the working version - I noticed that previously once the user note was added, the "user-notes-icon" element would be inserted next to the username. However I couldn't figure out the right model from within this closure to work with the previous code:
api.attachWidgetAction("post", "refreshUserNotes", function (count) {
const cfs = this.model.user_custom_fields || {};
cfs.user_notes_count = count;
this.model.set("user_custom_fields", cfs);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit which should fix your problem:
There are two parts in the fix:
The complexity of postAdminMenuButton
is that it has to work with widgets while not being a widget itself, and we also tried to not expose any widget API when working with postAdminMenuButton
to ensure we can keep moving away from widgets in the future without having to worry of people using widgets related function.
In the old code, sendWidgetAction
was called, which would ultimately trigger a re-render of widgets. postAdminMenuButton
is doing it for you automatically, but this case is special, because the actions is in two steps:
- you click the button to show the modal
- then you do the real action which should trigger a render, but
postAdminMenuButton
already did it at previous step
This is the implementation of clicking on a button in the post admin menu:
@action
async extraAction(button) {
await this.args.close();
await button.action(this.args.data.post);
await this.args.data.scheduleRerender();
}
So to fix this, I created a promise which will block await button.action(this.args.data.post);
and only be resolved when the count has been set, and then the flow will continue and call await this.args.data.scheduleRerender();
The other fix is to ensure we use the post
model instance, and not some POJO representation. This is a mistake of discourse codebase of mixing these when ideally we should only always use model instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this explains a lot, thanks!
Ideally trying to write a spec would be great, but this can be done in a followup to not stay in this broken state for too long. |
Original issue: https://meta.discourse.org/t/add-user-note-no-longer-shown-in-post-admin-options-menu/281957
Changes made in v3.2.0.beta2 resulted in a regression for the ability to attach a post-admin-menu-button with the widget API. This PR restores that functionality.
Testing:
fix.add.note.post.admin.button.mov