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(proposal): 404 error for co-speaker #1188

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

SivanYeh
Copy link
Collaborator

Types of changes

Thanks for sending a pull request! Please fill in the following content to let us know better about this change.
Please put an x in the box that applies

  • Bugfix
  • New feature
  • Refactoring
  • Breaking change (any change that would cause existing functionality to not work as expected)
  • Documentation Update
  • Other (please describe)

Description

Resolve 404 error and strange style of table after co-speaker pressed accept/decline during reviewing stage, which "SLUG.proposals.editable" is false.

Steps to Test This Pull Request

Steps to reproduce the behavior:

  1. Create 2 users: main speaker A and co speaker B.
  2. Go to Django admin and set to cfp stage as doc
  3. Sign in as speaker A into http://localhost:8000/en-us/dashboard/ and create a new proposal
  4. Go to Django admin and set to review stage.
  5. Found the proposal from speaker A and add B in "Additional Speakers" table.
  6. Sign in as speaker B into http://localhost:8000/en-us/dashboard/ and press "accept" and "decline" on the request.
  7. See error if page 404 shows up.

Expected behavior

404 happened in Step[7] disappear.

More Information

Screenshots
image

@SivanYeh
Copy link
Collaborator Author

@mattwang44 uncertain if it's a good solution: When I simply remove if/else statement in the dispatch function, 404 no longer shows up(of course) while speaker A remains unaffected(Did I miss anything?)

image
image

@SivanYeh SivanYeh requested review from mattwang44 and josix April 14, 2024 14:28
@mattwang44
Copy link
Member

記得修改 test cases

@SivanYeh
Copy link
Collaborator Author

記得修改 test cases

賀! 研究中

@SivanYeh SivanYeh closed this Apr 24, 2024
@SivanYeh SivanYeh force-pushed the fix_cfp_cospeaker branch from b7b01ac to b5de854 Compare April 24, 2024 09:13
Hide the form for co-speaker that contains buttons of decision Accepted/Declined selection.
@SivanYeh SivanYeh reopened this Apr 24, 2024
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.99%. Comparing base (fca6fd2) to head (a018685).
Report is 46 commits behind head on master.

Current head a018685 differs from pull request most recent head 6a6c2d5

Please upload reports for the commit 6a6c2d5 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1188      +/-   ##
==========================================
+ Coverage   71.19%   73.99%   +2.79%     
==========================================
  Files          84       81       -3     
  Lines        3451     3057     -394     
==========================================
- Hits         2457     2262     -195     
+ Misses        994      795     -199     

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

@SivanYeh SivanYeh requested a review from mattwang44 April 24, 2024 09:54
@@ -34,7 +34,7 @@
<td>{{ speaker_info.get_status_display }}</td>
<td>
<form method="post"
action="{% url 'additional_speaker_set_status' pk=speaker_info.pk %}">
action="{% url 'additional_speaker_set_status' pk=speaker_info.pk %}" hidden>
Copy link
Member

Choose a reason for hiding this comment

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

這個 form 永遠都會被藏起來,這是我們想要的嗎?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

果然答案沒有這麼簡單. 我再摸摸.
方向上是做一個switch?

Copy link
Member

@mattwang44 mattwang44 Apr 25, 2024

Choose a reason for hiding this comment

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

根據 discord 討論的 method 1,現況是後端已經有 switch(也就是 registry 存的 f'{settings.CONFERENCE_DEFAULT_SLUG}.proposals.editable'),方向是讓前端能根據這個 switch 現在是開或關來控制下列其一(可能需要先討論要選擇哪個):
a. 這個 form 上能夠調整共同講者狀態之按鈕的 disabled 與否
b. 這個 form 的顯示與否
c. 可能有其他做法?

This reverts commit 8eb2728.
Add switch on user_dashboard determined by the parameter "proposals_editable" in templates.
Add uneditable table and messages when editable parameter switches off.
Add test for additional speaker under uneditable condition.
@SivanYeh
Copy link
Collaborator Author

@mattwang44 I thought “request” can be one of parameters to switch the edit status and check if additional speaker is allowed to update/cancel the form during uneditable time.

...seems like “request” is not included in these two forms.
(Based on the par “request” came from “RequestUserValidationMixin”, which is not indcluded in “AdditionalSpeakerUpdateForm”.)

What is the key par should I consider?

@SivanYeh
Copy link
Collaborator Author

@mattwang44 跟GPT協作找到了一個方法是用RequestFactory來測試:
https://docs.djangoproject.com/en/5.0/topics/testing/advanced/
見commit54bf911

@mattwang44 mattwang44 self-requested a review July 30, 2024 04:53
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