Skip to content

feat: improve apply_to_all handling in upload forms #8680

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

peterthomassen
Copy link

  • Relabel apply_to_all field because old text caused confusion ("what is being applied?")
  • Set apply_to_all=False by default for uploading slides (as per the apparently common usecase)

@peterthomassen peterthomassen changed the title ApplyToAllFileUploadForm improvements feat: improve apply_to_all handling in upload forms Mar 16, 2025
@rjsparks
Copy link
Member

Hi Peter - thanks for this.
A question for future chair training - where did you run into the pain that led you to choose to contribute these changes? What can we do (beyond the changes you suggest here) to help others avoid that pain?

@peterthomassen
Copy link
Author

peterthomassen commented Mar 17, 2025

It was just that I (not a chair) was submitting slides, and asked our WG chair what that checkbox meant, because it wasn't clear to me. It was explained to me, together with the comment that they (the WG chairs) always untick that box during approval because slide decks usually go to only the session where they will be presented. So I thought it makes sense to set it to false initially.

As for what else to do, I think nothing is really needed beyond making clear what the checkbox means.

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.80%. Comparing base (70bf2ae) to head (cf1b6b8).
Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8680      +/-   ##
==========================================
- Coverage   88.88%   88.80%   -0.08%     
==========================================
  Files         312      314       +2     
  Lines       40891    41243     +352     
==========================================
+ Hits        36345    36627     +282     
- Misses       4546     4616      +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rjsparks
Copy link
Member

Please see https://mailarchive.ietf.org/arch/browse/wgchairs/?q=subject%3A(apply%20to%20all%20sessions)
I'll hold this PR for now, but it's likely that we'll close and not merge it, in favor of a refactor of the upload process instead that moves the checkbox to the approver rather than the submitter.

@peterthomassen
Copy link
Author

Fine with me. However,

  • if I'm not mistaken the checkbox is already on the approve form (see ApproveSlidesForm), so the task would be merely to remove it from the upload form (happy to adjust this PR accordingly);
  • the question remains open what the default value of this setting should be.

It seems to be that the default ideally should be a WG property, but my gut feeling is that that's unnecessarily complex.

@rjsparks
Copy link
Member

I'll shift this to draft as part of holding it aside.

@rjsparks rjsparks marked this pull request as draft March 27, 2025 15:12
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