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

Fixes removedAfterProceeding not dismissing final view when applied to final item in Workflow. #33

Merged
merged 29 commits into from Jun 9, 2021

Conversation

Richard-Gist
Copy link
Collaborator

@Richard-Gist Richard-Gist commented Jun 2, 2021

Linked Issue: Fixes #19

Checklist:


If Applicable:

  • Did you test when the first item is skipped?
  • Did you test when the last item is skipped?
  • Did you test when middle items are skipped?
  • Did you test when incorrect data is passed forward?
  • Did you test proceeding backwards?

If Public API Has Changed:

  • Did you deprecate (rather than remove) any old methods/variables/etc? Our philosophy for deprecation.
  • Have you done the best that you can to make sure that the compiler guides people to changing to the new API? (Example: the renamed attribute)
  • If necessary, have you tested the upgrade path for at least N-1 versions? For example, if data persists between v1 and v2 then that upgrade should be tested and as easy as we can make it.

@Richard-Gist Richard-Gist added the bug Something isn't working label Jun 2, 2021
@cla-bot cla-bot bot added the cla-signed label Jun 2, 2021
@Richard-Gist
Copy link
Collaborator Author

Richard-Gist commented Jun 2, 2021

This should have been a draft!!

EOD: We have decided to create a "complete" function on OrchestrationResponder and it will have the sole responsibility of calling the onFinish provided to the Workflow. In driving out these tests, we decided that the MockOrchestrationResponder should have the ability to call a default implementation. In this case, the default is that complete will call onFinish with the passedArgs.

These decisions are being worked through and the current state of the PR is incomplete for all of those decisions.

@Richard-Gist Richard-Gist marked this pull request as draft June 2, 2021 20:00
… calling the correct thing. Made it incorrect so we can still TDD it out. Also enabled swiftlint again - RAG
@wiemerm
Copy link
Contributor

wiemerm commented Jun 4, 2021

EOD: We addressed existing comments.

morganzellers
morganzellers previously approved these changes Jun 4, 2021
@wiemerm
Copy link
Contributor

wiemerm commented Jun 7, 2021

EOD: We're continuing with the solution that we had spiked on previously in handling calling onFinish() in our private destroy method.

morganzellers
morganzellers previously approved these changes Jun 8, 2021
@wiemerm wiemerm marked this pull request as ready for review June 8, 2021 18:58
Copy link
Collaborator Author

@Richard-Gist Richard-Gist left a comment

Choose a reason for hiding this comment

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

Also, we need to generate new docs for this.

Sources/WorkflowUIKit/UIKitPresenter.swift Outdated Show resolved Hide resolved
Tests/WorkflowTests/LinkedListTests.swift Outdated Show resolved Hide resolved
Sources/Workflow/Models/Workflow.swift Show resolved Hide resolved
Co-authored-by: Richard Gist <Richard.Gist@wwt.com>
@wiemerm wiemerm dismissed stale reviews from morganzellers via 41e83a5 June 8, 2021 19:29
…owExampleTests MockOrchestrationResponder - MW
@Richard-Gist Richard-Gist added the documentation Improvements or additions to documentation label Jun 8, 2021
@Richard-Gist Richard-Gist requested a review from a team as a code owner June 8, 2021 22:58
wiemerm
wiemerm previously approved these changes Jun 9, 2021
@wiemerm
Copy link
Contributor

wiemerm commented Jun 9, 2021

Good to merge after the docs are updated.

@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

86.7% 86.7% Coverage
0.0% 0.0% Duplication

@Richard-Gist Richard-Gist merged commit c651d74 into main Jun 9, 2021
@Richard-Gist Richard-Gist deleted the removedAfterProceeding branch June 9, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

removedAfterProceeding on last item in Workflow
4 participants