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

Detects the unsaved changes in the already confirmed steps. #537

Merged
merged 44 commits into from
Nov 8, 2023

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Oct 26, 2023

In the discussion with @unkcpz and @mikibonacci , we proposed a solution to handle the situation where the user edits the already confirmed steps.
This PR detects the unsaved changes in the already confirmed steps. If there are any unsaved changes, it will block the submit step and show the blocker messages there.

The technique details:

  • check the unsaved changes when the user leaves the step. This is achieved by observing the selected_index of the _wizard_app_widget, and comparing the related values (confirmed_structure in the step1, and configuration_parameters in step 2).
  • The _submission_blockers is added in the app and linked to the _app_submission_blockers in the submit step.

superstar54 and others added 21 commits September 23, 2023 07:07
additional modifications:

- changing visibility with display, for the codes. In this way, no extra
  blank space is allocated if a code is not visible. using the visibility
  attribute for the layout allocates it even if the code is hidden
…nput_structure.

This is needed to allow a dynamic display of the codes in step 3, if something is
changed in step 2.
@superstar54 superstar54 marked this pull request as draft October 26, 2023 16:23
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (251e423) 79.07% compared to head (0ec2601) 79.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
+ Coverage   79.07%   79.36%   +0.28%     
==========================================
  Files          47       47              
  Lines        3211     3251      +40     
==========================================
+ Hits         2539     2580      +41     
+ Misses        672      671       -1     
Flag Coverage Δ
python-3.10 79.36% <97.91%> (+0.28%) ⬆️
python-3.8 79.40% <97.91%> (+0.28%) ⬆️
python-3.9 79.40% <97.91%> (+0.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/aiidalab_qe/app/configuration/__init__.py 98.79% <100.00%> (+0.04%) ⬆️
src/aiidalab_qe/app/main.py 98.57% <100.00%> (+2.27%) ⬆️
src/aiidalab_qe/app/structure/__init__.py 98.63% <100.00%> (+0.03%) ⬆️
tests/test_app.py 100.00% <100.00%> (ø)
tests/test_codes.py 100.00% <ø> (ø)
src/aiidalab_qe/app/submission/__init__.py 79.50% <88.88%> (+0.16%) ⬆️

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

@superstar54 superstar54 marked this pull request as ready for review October 27, 2023 07:33
@superstar54
Copy link
Member Author

After a discussion with @unkcpz , we decided to make the following changes:

  • only when entering the submit step, do we update the blocker messages
  • select a new structure will partially reset the structure step, thus enabling the confirm button

We also agreed that in the submit step, updating the blocker_message inside the _update_date method is not a good way. I tried to separate it from the _update_date method, but I found that several other codes, e.g. qe_setup_status and sssp_installation_status also depend on this, and some logic and more codes need to update to make this happens, thus better to do this in another PR in the future. Anyway, for this PR, the current code works without a problem.

@unkcpz Please have another check.

@unkcpz
Copy link
Member

unkcpz commented Oct 28, 2023

Emm... no need to make complex changes actually. You can have a look at 5a775ea how the logic can be improved for the submission blocker message by separate the internal blocker and external blocker.
Can you adopt the changes from #539 to this PR and fix the unit test?

if change["new"]:
fmt_list = "\n".join((f"<li>{item}</li>" for item in sorted(change["new"])))
"""Observe the submission blockers and update the message area."""
blockers = self.internal_submission_blockers + self.external_submission_blockers
Copy link
Member

@unkcpz unkcpz Nov 7, 2023

Choose a reason for hiding this comment

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

  1. Is that possible that blockers are None then you cannot use +.
  2. The change is not used, please use _ in as argment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I change it to _change. The blockers are always list, I think we don't need to check None here.

Copy link
Member

Choose a reason for hiding this comment

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

If the variable is not used usually we use _ in widget callback.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @superstar54, some more requests. Should be all fine then, I'll give it a through test then.

@superstar54
Copy link
Member Author

@unkcpz Thanks for your review. I made the changes as you suggested. Please test it.


def _observe_selected_index(self, change):
"""Check unsaved change in the step when leaving the step."""
with self.submit_step.hold_sync():
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not needed, only at very end the submit_step is updated.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

@superstar54 superstar54 merged commit 0da5836 into main Nov 8, 2023
17 of 19 checks passed
@superstar54 superstar54 deleted the feature/edit_step branch November 8, 2023 14:01
unkcpz pushed a commit that referenced this pull request Dec 13, 2023
In #537 we introduce a mechanism that when the structure setup step or configure setup step is modified without confirmation, the submission step will be blocked. When loading finished calculations from the workflow selection bar, since setup steps are not confirmed, the user has to go back to previous steps to confirm which is not ideal. 
This PR make it automatically confirm the setup steps when the workflows are loaded from workflow selection dropdown.
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.

Critical ⚠️ : Reset structure after submit the workflow cause the break
3 participants