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 submission with "invite to proposal" status stuck in draft if language is not english #4220

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

theskumar
Copy link
Member

@theskumar theskumar commented Nov 16, 2024

The current implementation tries to find the transition based on the name property of the transitions available. The name of the transition is a translated string so while submitting a proposal the key "submit" from the request.POST could not be found in the available transition.

This PR updates the logic to fetch the transition target, by searching through the newly added marker on when to auto transition if the application is submitted.

Fixes #4211

See e6656d6 for patch.

Test Steps

  • Update the settings LANGUAGE_CODE to sv
  • For a fund with "Concept and Proposal" workflow, create a submission and approve the concept.
  • Now, logic as applicant and try to submit the "Proposal" while you application is in "Invited for Proposal" stage
  • The application should go through
  • If this patch is not applied, the status won't update.

@theskumar theskumar self-assigned this Nov 16, 2024
@theskumar theskumar added Type: Bug Bugs! Things that are broken :-/ Type: Patch Mini change, used in release drafter labels Nov 16, 2024
Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Excellent progress! This fixes the invite for proposal draft.

It does not seem to fix an initial draft, when e.g. an applicant apply for a lab or content note and begin with saving as draft.

hypha/apply/funds/views.py Show resolved Hide resolved
@theskumar
Copy link
Member Author

Excellent progress! This fixes the invite for proposal draft.

It does not seem to fix an initial draft, when e.g. an applicant apply for a lab or content note and begin with saving as draft.

It is a oversight from my side, the latest should work. def0e76

@theskumar
Copy link
Member Author

Test are failing, I'll take a deeper look.

@theskumar theskumar marked this pull request as draft November 18, 2024 03:32
@theskumar
Copy link
Member Author

After carefully reviewing, I realized that the application auto-advances to the next phase at different phases, and there could be multiple phases it can transition to when the application is at a specific phase, say more info required phase. Therefore, we need an explicit marker to decide:

  1. Whether to auto advance or not, if the user submits the application.
  2. which target phase to pick when the user submit a application.

The current fix, while functional, has an issue where it will always transition to the first available transition at that Phase.

I think we can solve this we need to update the workflow spec itself, that enables to deterministically know above two:

  1. Remove the translation marker, on the "Submit" display name, I believe it's not displayed in the UI anywhere.
  2. Introduce a new custom property on the target phase that indicates whether to auto-advance to this phase if the user submits the application at the source phase.

Screenshot 2024-11-18 at 3  32 04@2x

@frjo @wes-otf Any preference on the (1) or (2). My take would be to go for an explicit custom marker, even though it's little verbose, for the shake of clarity.

@sandeepsajan0
Copy link
Member

sandeepsajan0 commented Nov 18, 2024

@theskumar I am not sure about the best approach but what do you think about having a predefined method that can return both types of keys as translated(in same lang) and we can call that method wherever in code we are comparing these keys like here.
Ex:

def translated_keys(post_keys, transition_keys):
    ......
    return translated_post_keys, translated_transition_keys

@theskumar
Copy link
Member Author

theskumar commented Nov 18, 2024

@theskumar I am not sure about the best approach but what do you think about having a predefined method that can return both types of keys as translated(in same lang) and we can call that method wherever in code we are comparing these keys like here. Ex:

def translated_keys(post_keys, transition_keys):
    ......
    return translated_post_keys, translated_transition_keys

Thank for the input, initial tried getting the untranslated version of the display but it looks by the time it reaches here as a key of the dictionaries, this is already translated, and it make sense as the translated string will be needed for hashing in in the dict.

I'm in a opinion that we should not be doing comparison on the basis of translated strings.

Also, since the post.keys are never marked as translatable, you can't get equivalent translated string reliably.

@frjo
Copy link
Contributor

frjo commented Nov 18, 2024

I think option two is best.

I agree that we should never compare strings that might be translated.

I see no other places but in this form_valid() function that we do this check. Correct?

Progressing when saving a submission should only happen when submitting a draft and when submitting a "More information is needed".

Having something explicit in the workflows for these two statuses would make it more clear what is happening.

@theskumar
Copy link
Member Author

I see no other places but in this form_valid() function that we do this check. Correct?

Yes, that's when we check if the "Submit" action is present in the targets and then perform_transition to that phase.

I'll check other places as well, but it doesn't seem it should have been used. (If we are using translated string for comparisons at other places, that would probably be another ticket for us to go over)


For the custom route, I'll need to modify some parts of how the phases and transitions are initialized, but it shouldn't be too complicated. I'll be updating the this PR.

PS: Down the road, we must split this workflow file into different module. I see two Request object defined in the same file, one related to Stage and another for Workflow.

@theskumar theskumar marked this pull request as ready for review November 20, 2024 06:17
@theskumar theskumar requested a review from frjo November 20, 2024 06:18
@theskumar
Copy link
Member Author

Updated the logic to add new marker in the workflow definition indicating this particular phase to pick when the user submits the application and then auto transition to it.

Copy link
Contributor

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Works nicely for me.

@frjo frjo added Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team labels Nov 20, 2024
@wes-otf
Copy link
Contributor

wes-otf commented Nov 25, 2024

worked great, really nice work @theskumar!

@wes-otf wes-otf added Status: Tested - approved for live ✅ and removed Status: Needs testing Tickets that need testing/qa Status: Needs dev testing 🧑‍💻 Tasks that should be tested by the dev team labels Nov 25, 2024
@frjo frjo merged commit 02398cd into main Nov 25, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Tested - approved for live ✅ Type: Bug Bugs! Things that are broken :-/ Type: Patch Mini change, used in release drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal stuck in draft when language code is set
4 participants