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

Fix/allow multiple notes #182

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Fix/allow multiple notes #182

wants to merge 20 commits into from

Conversation

cbkerr
Copy link
Member

@cbkerr cbkerr commented Apr 3, 2023

Description

Uses Blueprints to differentiate between module endpoints in the Notes module. I also consider this a prototype to use Blueprints for all modules so that you can enable multiple of them.

There is still a problem where it runs the endpoint notes_update as many times as there are instances of the Notes module. This problem relates to registering the module assets.
If I comment out the part about dashboard.register_module_asset(...), it only runs the endpoint once, but you have to manually go back to the page because you end up on the URL of the action.

Motivation and Context

Resolves #74
image

Checklist:

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #182 (e462385) into main (81981c1) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   69.98%   70.09%   +0.10%     
==========================================
  Files          21       21              
  Lines         883      886       +3     
  Branches      159      159              
==========================================
+ Hits          618      621       +3     
  Misses        220      220              
  Partials       45       45              
Files Changed Coverage Δ
signac_dashboard/modules/notes.py 71.42% <100.00%> (+2.67%) ⬆️

@cbkerr cbkerr mentioned this pull request Apr 15, 2023
5 tasks
@bdice
Copy link
Member

bdice commented Apr 23, 2023

@cbkerr It looks like this PR may need to be updated a bit. It appears to have commits from #181. It might be that this is in a problematic state due to the master/main rename, and changing the draft state or target branch may force it to refresh. You could also rebase on main and force-push, I think. Let me know when this is ready for review, when you open the draft.

edit: Seems like maybe this fixed itself?

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@cbkerr Can you clean up all the print / debugging here? Happy to review again after that.

@cbkerr cbkerr marked this pull request as ready for review August 18, 2023 15:27
@cbkerr cbkerr requested review from a team as code owners August 18, 2023 15:27
@cbkerr cbkerr requested review from vyasr and lyrivera and removed request for a team August 18, 2023 15:27
@cbkerr cbkerr requested a review from bdice August 18, 2023 15:27
@bdice
Copy link
Member

bdice commented Aug 24, 2023

There is still a problem where it runs the endpoint notes_update as many times as there are instances of the Notes module. This problem relates to registering the module assets.

I'm a bit concerned about merging this PR with a design flaw like this. @cbkerr Do you see a way forward for this?

@bdice
Copy link
Member

bdice commented Sep 19, 2023

@cbkerr Ping - just checking to see if you have thoughts about the registration problem. Do you think this is a blocker? Or should we merge this PR regardless, and file an issue?

@bdice
Copy link
Member

bdice commented Sep 25, 2023

Removing review requests due to inactivity.

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.

Only 1 Notes instance appears on dashboard
2 participants