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] Remember the last selected payer for each project #1224

Merged
merged 3 commits into from Oct 2, 2023

Conversation

Jojo144
Copy link
Contributor

@Jojo144 Jojo144 commented Sep 7, 2023

Previously the last selected payer was lost when switching of project.

The structure of the variable last_selected_payer is changed (now it's a dictionary) then it has been renamed to last_selected_payer_per_project to avoid session bugs.

@almet
Copy link
Member

almet commented Sep 8, 2023

Hey, thanks for the changes.

Could you make some edits so that it's not lost for all users when we release the new version? You could do that by checking if last_selected_payer exists, and if it does use it to populate the new dict.

Also, could you add some tests about this, to ensure it's working the way it's intended? Thanks :-)

@Jojo144
Copy link
Contributor Author

Jojo144 commented Oct 1, 2023

Could you make some edits so that it's not lost for all users when we release the new version? You could do that by checking if last_selected_payer exists, and if it does use it to populate the new dict.

@almet Are you sure of that? 99.9 % won't remark that the default user is lost during the update and it will produce dead code...

I have a look at the tests.

@almet
Copy link
Member

almet commented Oct 1, 2023

We can mark the code for deletion later with a comment, it's not an issue.

Thanks :-)

@zorun
Copy link
Collaborator

zorun commented Oct 1, 2023

I also don't think we need compatibility on this one, especially because it was already lost when switching project. But now it's done...

@Jojo144 you also need to rebase that one, sorry. Can you also adapt your test to use plain asserts?

@azmeuk
Copy link
Contributor

azmeuk commented Oct 2, 2023

In the meantime #1213 has been merged. Can you use pytest assertions in unit tests?

@Jojo144
Copy link
Contributor Author

Jojo144 commented Oct 2, 2023

Done. Thanks for your comment.

@zorun
Copy link
Collaborator

zorun commented Oct 2, 2023

Thanks!

@zorun zorun merged commit b92a36c into spiral-project:master Oct 2, 2023
15 checks passed
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.

None yet

4 participants