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

[GH-20818] - Add restore run feature for playbook #1448

Merged
merged 9 commits into from
Sep 1, 2022

Conversation

rolwin100
Copy link
Contributor

@rolwin100 rolwin100 commented Aug 28, 2022

Summary

  • This feature restores a run that is finished.
  • There was a /restore feat already present in the repo
  • created a api request in client.ts
  • added a hook in restore_run.tsx
  • Added menu in button in context_menu.tsx and called the hook on button click.

Ticket Link

mattermost/mattermost#20818

Checklist

  • Telemetry updated
  • Gated by experimental feature flag
  • Unit tests updated

* There was a /restore feat already present in the repo
* created a api request in client.ts
* added a hook in restore_run.tsx
* Added menu in button in context_menu.tsx and called the hook on button click.

Resolves #20818 - on github
@mattermod
Copy link
Contributor

Hello @rolwin100,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Copy link
Contributor

@nickmisasi nickmisasi left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Great work on @rolwin100!! 🎉

Couple changes required.

My question here is whether this is restoring or restarting the run. Will the run pick up where it was left off (ie, any already completed steps will still be completed) or will this act more like it's a fresh new run?

Depending on which this is, I think it's important to ensure that all verbiage (things displayed to user, as well as file names/function names/etc) is consistently either restore or restart to avoid confusion.

webapp/i18n/en.json Outdated Show resolved Hide resolved
@itao
Copy link
Contributor

itao commented Aug 29, 2022

👋 Hi @rolwin100, thanks for taking on this PR! I noticed that the original issue mentioned

Restarting a run should add an event to the timeline, as well as post a message to the associated channel:

Could you confirmed if that was something already implemented or if it's expected as part of this PR, please?

@rolwin100
Copy link
Contributor Author

rolwin100 commented Aug 30, 2022

@itao Its'a part of this PR.

@itao
Copy link
Contributor

itao commented Aug 30, 2022

@nickmisasi regarding your question whether this is restoring or restarting the run.

Yes, the run will pick up where it was left off. However I prefer the copy restart over restore since the latter is more often associated with archiving or deletion, whereas the action we're undoing in this case is finishing. There is still a difference between:

  • restart the run: pick up where it left off when marked as finished
  • rerun the playbook: start a fresh new run

@rolwin100
Copy link
Contributor Author

rolwin100 commented Aug 30, 2022

@nickmisasi regarding your question whether this is restoring or restarting the run.

Yes, the run will pick up where it was left off. However I prefer the copy restart over restore since the latter is more often associated with archiving or deletion, whereas the action we're undoing in this case is finishing. There is still a difference between:

  • restart the run: pick up where it left off when marked as finished
  • rerun the playbook: start a fresh new run

@itao @nickmisasi can we proceed with the existing change as of now if it is okay with you people because the API which was present was named restore and I don't see much problem with this.

What do you guys think?

Copy link
Contributor

@gbochora gbochora left a comment

Choose a reason for hiding this comment

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

Great work, @rolwin100! Thanks! I left two optional suggestions.

@gbochora
Copy link
Contributor

gbochora commented Sep 1, 2022

@rolwin100, great work on tests; thanks again! We will be able to merge this PR after the second reviewer approves it.

Copy link
Contributor

@nickmisasi nickmisasi left a comment

Choose a reason for hiding this comment

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

Code LGTM. Will need tests to pass before approval. Thanks @rolwin100 🎉!

Copy link
Contributor

@nickmisasi nickmisasi left a comment

Choose a reason for hiding this comment

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

Thanks @rolwin100!

@Phrynobatrachus Phrynobatrachus merged commit 25b245c into mattermost:master Sep 1, 2022
@rolwin100 rolwin100 deleted the feature/restore_run branch September 2, 2022 04:47
@rolwin100 rolwin100 restored the feature/restore_run branch September 2, 2022 04:47
@rolwin100 rolwin100 deleted the feature/restore_run branch September 2, 2022 04:47
trilopin added a commit that referenced this pull request Sep 5, 2022
* master: (25 commits)
  migrate fa icons to compass + fix pb assign owner clear (#1462)
  [GH-20818] - Add restore run feature for playbook (#1448)
  datetime selector: set custom error text (#1459)
  remove negative letter spacing and set wider min-width for context menu at PBE (#1453)
  Fix playbooks list refresh issue (#1436)
  RHS: do not trigger inline edit if user is doing a text-selection (#1452)
  remove rounded edges in add-new-task button (#1450)
  MM-45933: Playbook List Columns: Last used, Active runs, Runs in 30 days (#1360)
  add hover background color to pbe broadcast channel selector (#1451)
  Switch model for versioning golangci-lint (#1425)
  [MM-44815] Duplicate task as next task instead of the last task (#1430)
  [MM-45741] Add following button to runs list (#1370)
  Mm 46379 fav run in LHS queries (#1437)
  fix type for globalstate -> fix CI (#1447)
  Add channel scheme role support (#1431)
  avoid requiring permission for leaving a playbook (#1439)
  [MM-37996] create the new modal UI for the playbook run dialog (#1245)
  MM-43277: fix channel actions modal dropdown cutoffs/overflows (#1418)
  Translated using Weblate (Swedish)
  Translated using Weblate (Swedish)
  ...
trilopin added a commit that referenced this pull request Sep 7, 2022
* master:
  migrate md icons  to compass (#1460)
  Fix OpenAPI spec, so it validates as OpenAPI v3; Switch http links to https links in schema examples (#1438)
  Mm 44752 add dot menu to lhs (#1458)
  migrate fa icons to compass + fix pb assign owner clear (#1462)
  [GH-20818] - Add restore run feature for playbook (#1448)
  datetime selector: set custom error text (#1459)
  remove negative letter spacing and set wider min-width for context menu at PBE (#1453)
  Fix playbooks list refresh issue (#1436)
  RHS: do not trigger inline edit if user is doing a text-selection (#1452)
  remove rounded edges in add-new-task button (#1450)
  MM-45933: Playbook List Columns: Last used, Active runs, Runs in 30 days (#1360)
  add hover background color to pbe broadcast channel selector (#1451)
  Switch model for versioning golangci-lint (#1425)
  [MM-44815] Duplicate task as next task instead of the last task (#1430)
  [MM-45741] Add following button to runs list (#1370)
trilopin added a commit that referenced this pull request Sep 7, 2022
* master:
  Separate run participants base (#1440)
  Tsprune (#1461)
  migrate md icons  to compass (#1460)
  Fix OpenAPI spec, so it validates as OpenAPI v3; Switch http links to https links in schema examples (#1438)
  Mm 44752 add dot menu to lhs (#1458)
  migrate fa icons to compass + fix pb assign owner clear (#1462)
  [GH-20818] - Add restore run feature for playbook (#1448)
trilopin added a commit that referenced this pull request Sep 8, 2022
* master:
  limit size for all incoming requests and migrate postgres text column  (#1463)
  wait() more :| (#1468)
  [MM-45662] Fix context dropdown closing when clicked (#1467)
  Separate run participants base (#1440)
  Tsprune (#1461)
  migrate md icons  to compass (#1460)
  Fix OpenAPI spec, so it validates as OpenAPI v3; Switch http links to https links in schema examples (#1438)
  Mm 44752 add dot menu to lhs (#1458)
  migrate fa icons to compass + fix pb assign owner clear (#1462)
  [GH-20818] - Add restore run feature for playbook (#1448)
  datetime selector: set custom error text (#1459)
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

6 participants