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

Export specific workflow #11939

Merged
merged 69 commits into from
Apr 25, 2024
Merged

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Jul 2, 2022

Export the specified workflow from the workflow management interface

image
export 1

@hyzx86 hyzx86 requested a review from sfmskywalker as a code owner July 2, 2022 14:48
@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 2, 2022

@Skrypt Can you review this PR?

@hishamco
Copy link
Member

hishamco commented Jul 2, 2022

Could you please share a GIF screenshot shows how to export the workflow and import it then

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 2, 2022

msedge_FMw0XBFaQ1

@hyzx86
Copy link
Contributor Author

hyzx86 commented Jul 2, 2022

try mp4 file

msedge_qghN7nTjmh.mp4

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

Please format the code, there are many things to fix

@@ -127,7 +131,7 @@ public async Task<IActionResult> Index(WorkflowTypeIndexOptions options, PagerPa

var workflowTypeIds = workflowTypes.Select(x => x.WorkflowTypeId).ToList();
var workflowGroups = (await _session.QueryIndex<WorkflowIndex>(x => x.WorkflowTypeId.IsIn(workflowTypeIds))
.ListAsync())
.ListAsync())
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space

Copy link
Member

Choose a reason for hiding this comment

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

Still not reeolved, there's double tabs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strangely, I have fixed this problem on my branch, but it is still shown here

image

var deploymentPlanResult = new DeploymentPlanResult(fileBuilder, recipeDescriptor);
var data = new JArray();
deploymentPlanResult.Steps.Add(new JObject(
new JProperty("name", "WorkflowType"),
Copy link
Member

Choose a reason for hiding this comment

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

Add intendation

foreach (var workflow in await _workflowTypeStore.GetAsync(itemIds))
{
var objectData = JObject.FromObject(workflow);

Copy link
Member

Choose a reason for hiding this comment

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

Remove the empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

objectData.Remove(nameof(workflow.Id));
data.Add(objectData);
}
await deploymentPlanResult.FinalizeAsync();
Copy link
Member

Choose a reason for hiding this comment

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

Add new line before

}
await deploymentPlanResult.FinalizeAsync();
ZipFile.CreateFromDirectory(fileBuilder.Folder, archiveFileName);
return archiveFileName;
Copy link
Member

Choose a reason for hiding this comment

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

Add new line before

await deploymentPlanResult.FinalizeAsync();
ZipFile.CreateFromDirectory(fileBuilder.Folder, archiveFileName);
return archiveFileName;

Copy link
Member

Choose a reason for hiding this comment

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

Remove empty line

@@ -207,6 +218,44 @@ public async Task<IActionResult> BulkEdit(WorkflowTypeIndexOptions options, IEnu
return RedirectToAction(nameof(Index));
}

private async Task<string> ExportWorkflows(params int[] itemIds)
Copy link
Member

Choose a reason for hiding this comment

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

Remove the private methods at the end

{
return Forbid();
}
var resultFileName = await ExportWorkflows(id);
Copy link
Member

Choose a reason for hiding this comment

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

Add line before

@hyzx86 hyzx86 requested a review from hishamco July 3, 2022 05:12
Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

Please double check the format for the entire PR before you request a review to save time

Thanks for your contribution again

@@ -127,7 +131,7 @@ public async Task<IActionResult> Index(WorkflowTypeIndexOptions options, PagerPa

var workflowTypeIds = workflowTypes.Select(x => x.WorkflowTypeId).ToList();
var workflowGroups = (await _session.QueryIndex<WorkflowIndex>(x => x.WorkflowTypeId.IsIn(workflowTypeIds))
.ListAsync())
.ListAsync())
Copy link
Member

Choose a reason for hiding this comment

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

Still not reeolved, there's double tabs here

: Url.IsLocalUrl(viewModel.ReturnUrl)
? (IActionResult)this.Redirect(viewModel.ReturnUrl, true)
: RedirectToAction(nameof(Index));
? (IActionResult)this.Redirect(viewModel.ReturnUrl, true)
Copy link
Member

Choose a reason for hiding this comment

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

Extra space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original version of this place has 3 spaces indented, and the automatic indent should be 4, right?

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 space is extra, not resolved yet!

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 space is extra, not resolved yet!

@@ -46,6 +46,7 @@
@T["Actions"]
</button>
<ul class="dropdown-menu dropdown-menu-end" aria-labelledby="bulk-action-menu-button">
<li><a class="dropdown-item" href="javascript:void(0)" data-action="Export" data-title="@T["Bulk Action"]" >Export</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

Seems there's n extra space ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are we going to do here?

@hyzx86 hyzx86 requested a review from hishamco July 3, 2022 15:16
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but almost done.

Copy link
Contributor

coderabbitai bot commented Apr 25, 2024

@hyzx86, I've reviewed the changes related to the UserClaims processing logic and the associated unit test. Here are my observations:

Processing Logic for UserClaims

The processing logic for UserClaims is implemented in the DefaultUserClaimsPrincipalProviderFactory class. This class is marked as obsolete, suggesting a transition to using IUserClaimsProvider instead. The logic includes generating claims for the user's email and whether the email is verified. This is done by adding claims directly to the ClaimsIdentity object after fetching the email and its verification status from the user manager.

Unit Test

The unit test EnsurePrincipalHasExpectedClaims in DefaultUserClaimsPrincipalProviderFactoryTests checks the correctness of the claims added to the principal. It verifies:

  • The presence of the email claim and its correctness.
  • The presence of the email_verified claim, its type, and its correctness based on whether the email is verified.

Both the processing logic and the unit test appear to be correctly implemented based on the provided snippets. The test covers essential aspects of the functionality, ensuring that the claims are correctly added and have the expected values.

If there are no other specific areas you would like me to look into, this PR seems ready for further actions as per your project's workflow.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Find and display the changes related to `UserClaims` processing logic and the associated unit test.
# Assuming the changes might be in files related to user management or authentication, typically involving 'Claims'.

# Search for files that might contain changes related to UserClaims
fd 'UserClaims' --type f --exec cat {} +

# Search for unit tests possibly related to UserClaims
fd 'UserClaims' --type f --exec cat {} + | grep -i 'test'

Length of output: 6432

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

@hishamco up to you, please approve/merge if suitable.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

@Piedone
Copy link
Member

Piedone commented Apr 25, 2024

@hishamco when you see this, you can just clone the fork and commit to it directly, no need to suffer through self-approved code suggestions.

image

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

private readonly IUpdateModelAccessor _updateModelAccessor;
private readonly IShapeFactory _shapeFactory;
private readonly JsonSerializerOptions _documentJsonSerializerOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

The field _documentJsonSerializerOptions should be renamed to _jsonSerializerOptions to match its usage throughout the class.

- private readonly JsonSerializerOptions _documentJsonSerializerOptions;
+ private readonly JsonSerializerOptions _jsonSerializerOptions;

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
private readonly JsonSerializerOptions _documentJsonSerializerOptions;
private readonly JsonSerializerOptions _jsonSerializerOptions;

IStringLocalizer<WorkflowTypeController> stringLocalizer,
IHtmlLocalizer<WorkflowTypeController> htmlLocalizer,
IUpdateModelAccessor updateModelAccessor)
IUpdateModelAccessor updateModelAccessor,
IOptions<DocumentJsonSerializerOptions> jsonSerializerOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

The constructor parameter should be named documentJsonSerializerOptions to match the field it initializes.

- IOptions<DocumentJsonSerializerOptions> jsonSerializerOptions)
+ IOptions<DocumentJsonSerializerOptions> documentJsonSerializerOptions)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
IOptions<DocumentJsonSerializerOptions> jsonSerializerOptions)
IOptions<DocumentJsonSerializerOptions> documentJsonSerializerOptions)

_updateModelAccessor = updateModelAccessor;
_shapeFactory = shapeFactory;
S = stringLocalizer;
H = htmlLocalizer;
_documentJsonSerializerOptions = jsonSerializerOptions.Value.SerializerOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

The assignment to _documentJsonSerializerOptions should use the corrected field name _jsonSerializerOptions.

- _documentJsonSerializerOptions = jsonSerializerOptions.Value.SerializerOptions;
+ _jsonSerializerOptions = documentJsonSerializerOptions.Value.SerializerOptions;

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
_documentJsonSerializerOptions = jsonSerializerOptions.Value.SerializerOptions;
_jsonSerializerOptions = documentJsonSerializerOptions.Value.SerializerOptions;

@@ -192,6 +198,9 @@ public async Task<IActionResult> BulkEdit(WorkflowTypeIndexOptions options, IEnu
{
case WorkflowTypeBulkAction.None:
break;
case WorkflowTypeBulkAction.Export:
return await ExportWorkflows(itemIds.ToArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid duplicate calls to ExportWorkflows. Ensure that the method is called only once per action to prevent performance issues.

- return await ExportWorkflows(itemIds.ToArray());

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
return await ExportWorkflows(itemIds.ToArray());

Comment on lines +226 to +234
public async Task<IActionResult> Export(int id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageWorkflows))
{
return Forbid();
}

return await ExportWorkflows(id);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The Export method should be decorated with the [HttpGet] attribute since it is handling a GET request, not a POST.

+ [HttpGet]
public async Task<IActionResult> Export(int id)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
public async Task<IActionResult> Export(int id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageWorkflows))
{
return Forbid();
}
return await ExportWorkflows(id);
}
[HttpGet]
public async Task<IActionResult> Export(int id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageWorkflows))
{
return Forbid();
}
return await ExportWorkflows(id);
}

@hishamco
Copy link
Member

@hishamco when you see this, you can just clone the fork and commit to it directly, no need to suffer through self-approved code suggestions.

I remember one time ago I accidentally overridden someone else commits :(

@hishamco hishamco merged commit f15ccb7 into OrchardCMS:main Apr 25, 2024
4 checks passed
@hyzx86 hyzx86 deleted the Export_specified_Workflow branch May 13, 2024 08:41
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.

5 participants