search

Lombiq / Open-Source-Orchard-Core-Extensions

Looking for some useful Orchard Core extensions? Here's a bundle solution of all of Lombiq's open-source Orchard Core extensions (modules and themes). Clone and try them out!
https://lombiq.com
BSD 3-Clause "New" or "Revised" License
47 stars 19 forks source link

OSOE-751: Upgrade to Orchard Core 1.8 #673

Closed Psichorex closed 7 months ago

Psichorex commented 9 months ago

OSOE-751 Fixes #638

coderabbitai[bot] commented 9 months ago

Walkthrough

This update primarily focuses on upgrading to Orchard Core 1.8, aligning with the latest Orchard Core improvements and API changes. It involves updates to target frameworks, package versions, and subproject commit references across various modules, utilities, and test projects. Additionally, there's a concerted effort to improve asynchronous calls and integrate new Orchard Core features, reflecting a broad enhancement of the project's functionality and compatibility with the latest Orchard Core version.

Changes

Files or Directories Change Summary
src/.../Lombiq.OSOCE.Web.csproj, test/.../Lombiq.OSOCE.Tests.UI.csproj Updated TargetFramework to net8.0 and incremented package versions.
src/Modules/..., src/Libraries/..., tools/... Updated subproject commit references.
.github/workflows/... Updated GitHub Actions workflow references.
test/.../Tests/ModuleTests/... Modified constructors to accept ITestOutputHelper and added/modified test assertions.

Assessment against linked issues

Objective Addressed Explanation
Upgrade to Orchard Core 1.8 (OSOE-751) [#638] ✅
Utilize new async features from Orchard Core PRs [#638] ❓ The summary does not explicitly mention the utilization of new async features from specified Orchard Core PRs. However, the upgrade to Orchard Core 1.8 likely encompasses these changes indirectly.
Update forms according to Orchard Core PR [#638] ❓ Without specific mention of form updates in the summary, it's unclear if the changes from Orchard Core PR #14806 were applied.
Add release notes about AI setup changes and UI Testing Toolbox [#638] ❌ The provided summary does not include information about updating documentation or release notes.

The overall changes seem to align well with the primary objective of upgrading to Orchard Core 1.8, with some specifics about how new async features and form updates from Orchard Core PRs have been applied remaining unclear from the summary alone. Documentation updates, particularly for release notes, appear to be outside the scope of the provided changes.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with CodeRabbit: - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit-tests for this file.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit tests for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository from git and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (2)

contentpreview teszt

Previously acknowledged words that are now absent listheader mediatheme notlike npmrc PII Portainer prebuild taxonomyfield visualstudioextension vuejs 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands ... in a clone of the [git@github.com:Lombiq/Open-Source-Orchard-Core-Extensions.git](https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions.git) repository on the `issue/OSOE-751` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/Lombiq/GitHub-Actions/dev/apply.pl' | perl - 'https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7687987650/attempts/1' ```
Available :books: dictionaries could cover words not in the :blue_book: dictionary Dictionary | Entries | Covers | Uniquely -|-|-|- [cspell:typescript/dict/typescript.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/typescript/dict/typescript.txt)|1098|1|1| Consider adding them (in `.github/workflows/build-and-test.yml`) to `extra_dictionaries`: ``` yml cspell:typescript/dict/typescript.txt ``` To stop checking additional dictionaries, add (in `.github/workflows/build-and-test.yml`): ``` yml check_extra_dictionaries: '' ```
github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (1)

Asnyc

Previously acknowledged words that are now absent listheader mediatheme notlike npmrc PII Portainer prebuild taxonomyfield visualstudioextension vuejs 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands ... in a clone of the [git@github.com:Lombiq/Open-Source-Orchard-Core-Extensions.git](https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions.git) repository on the `issue/OSOE-751` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/Lombiq/GitHub-Actions/dev/apply.pl' | perl - 'https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7698918736/attempts/1' ```
Available :books: dictionaries could cover words not in the :blue_book: dictionary Dictionary | Entries | Covers | Uniquely -|-|-|- [cspell:typescript/dict/typescript.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/typescript/dict/typescript.txt)|1098|1|1| Consider adding them (in `.github/workflows/build-and-test.yml`) to `extra_dictionaries`: ``` yml cspell:typescript/dict/typescript.txt ``` To stop checking additional dictionaries, add (in `.github/workflows/build-and-test.yml`): ``` yml check_extra_dictionaries: '' ```
github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (1)

execption

Previously acknowledged words that are now absent listheader mediatheme notlike npmrc PII Portainer prebuild taxonomyfield visualstudioextension vuejs 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands ... in a clone of the [git@github.com:Lombiq/Open-Source-Orchard-Core-Extensions.git](https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions.git) repository on the `issue/OSOE-751` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/Lombiq/GitHub-Actions/dev/apply.pl' | perl - 'https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7700174720/attempts/1' ```
Available :books: dictionaries could cover words not in the :blue_book: dictionary Dictionary | Entries | Covers | Uniquely -|-|-|- [cspell:typescript/dict/typescript.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/typescript/dict/typescript.txt)|1098|1|1| Consider adding them (in `.github/workflows/build-and-test.yml`) to `extra_dictionaries`: ``` yml cspell:typescript/dict/typescript.txt ``` To stop checking additional dictionaries, add (in `.github/workflows/build-and-test.yml`): ``` yml check_extra_dictionaries: '' ```
github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

Psichorex commented 8 months ago

https://github.com/OrchardCMS/OrchardCore/issues/15222

Piedone commented 8 months ago

Always add such issues links for workarounds in the code. And keep to https://lombiq.atlassian.net/wiki/spaces/OCORE/overview#Creating-issues (I added it to our GitHub project now).

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

Psichorex commented 8 months ago

https://github.com/Lombiq/NodeJs-Extensions/actions/runs/7887385794 This happens when trying to publish to NuGet. As far as I am concerned collection literals are not a preview feature. I didn't find a corresponding part of the https://lombiq.atlassian.net/wiki/spaces/DEV/pages/786857987/Managing+releases+of+open-source+projects whether what to do if a .NET upgrade happens for instance. Do you have any idea or suggestions how to overcome this?

Piedone commented 8 months ago

NE targets netstandard2.0 and should keep doing that for compatibility with .NET Framework projects. You can't use new C# features there.

Psichorex commented 8 months ago

NE targets netstandard2.0 and should keep doing that for compatibility with .NET Framework projects. You can't use new C# features there.

That's what I was going to ask this moment. Whether NE actually brings the langVersion from the global setting. BUT this error is not present locally. Which means if I don't use collection literals it will induce warnings in the solution. So shall we supress these warnings of manipulate that rule?

Piedone commented 8 months ago

You can also specify <langVersion> just in the csproj of NE, with a comment on why that's necessary, to avoid the warnings.

Psichorex commented 8 months ago

The default <LangVersion> for netstandard2.0 is C# 7.3. But that also doesn't include file scoped namespaces. So what language version shall we use there? Btw namespaces were introduced in C# 10 which requires .NET 6 so I don't even understand how is that usable in a netstandard2.0 project and the CI didn't warn us before about that one.

Piedone commented 8 months ago

Please use what works with netstandard2.0. But I don't think you need to do a release here, since only the sample is updated, and this project doesn't depend on OC.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

Psichorex commented 8 months ago

Please use what works with netstandard2.0. But I don't think you need to do a release here, since only the sample is updated, and this project doesn't depend on OC.

I adjusted the syntaxes used in Lombiq.NodeJs.Extensions you can see the changes in the the PR:https://github.com/Lombiq/NodeJs-Extensions/pull/91/files#diff-9cf2858bf3bd132496ec78748544fe8bb1e628b8244b6e65be188fa6f774b5de But as you said in this case I won't actually do a release here because only the .Tests project is modified by the .NET upgrade and it is OC independent.

Piedone commented 8 months ago

Thanks!

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

Psichorex commented 8 months ago

CI Tests are failing due to this: image

I will increase the timeout now but I have a feeling that this shouldn't be increased. Does .NET 8 have maybe anything to do with increased timeout demands? We need them both on windows and ubuntu now on the larger runners. I am not an expert in this field I haven't yet looked into timeout values and why we need them. All I know is that you have mentioned it should be less than 720000 on larger runners.

Piedone commented 8 months ago

That timeout is already large and shouldn't be increased without adding any further tests. Most possibly a test is flaky or downright failing.

Psichorex commented 8 months ago

That timeout is already large and shouldn't be increased without adding any further tests. Most possibly a test is flaky or downright failing.

Maybe related: https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/pull/694 but here nothing was touched that should cause such a case. It was derived from latest stable dev and only a PS1 script was changed.

Psichorex commented 8 months ago

Also this: https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7905683913?pr=673 I see that this was recently fixed but it seems that the fix doesn't cover all cases. Correspoding issue is OSOE-771https://lombiq.atlassian.net/jira/software/c/projects/OSOE/boards/36?selectedIssue=OSOE-771

Piedone commented 8 months ago

So, it's indeed a failing test. You need to fix that (looks like an incompatibility that 1.8 brought) and revert the timeout values.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

Psichorex commented 8 months ago

Do you know how to increase the test case timeout? https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7914125012/attempts/1#ra3eb2a264s18 Walkthorugh tests are timing out and I am not getting a proper error why is it actually failing. For me locally the test passes at the first run.

Piedone commented 8 months ago

This is a different timeout, some browser command doesn't complete, i. e. the test completes but fails. See the stack trace in the error message and download the failure dump.

Psichorex commented 8 months ago

This is a different timeout, some browser command doesn't complete, i. e. the test completes but fails. See the stack trace in the error message and download the failure dump.

The part that always fails is somewhere at the Deployment plan section. I now went thorugh the logs and found the following: image This part is recursively appearing until the test times out. And by the screenshot it uploads it seems that the popup modal is appearing in such a way that it is covering the select button of the OC modal's card element thus I can't click on it gets stuck? 64

Piedone commented 8 months ago

Then, apparently that UI changed in 1.8 and you have to change the selector the Walkthroughs module uses to display the tooltip not to hide it.

Psichorex commented 8 months ago

Then, apparently that UI changed in 1.8 and you have to change the selector the Walkthroughs module uses to display the tooltip not to hide it.

The main problem is that he javascript that the module uses to render that tooltip is fine. Locally it is working as it should be. See:

javascript: image Local data-popper-placement="top": image

CI LOGS

data-popper-placement="bottom" image image

At this point I have no clue why does the CI env change the render position from top to bottom as the script clearly says it should be top. I checked the button element on both CI and Local and they are exactly the same. Local: image CI: image

Psichorex commented 8 months ago

I changed it from top to right so that window size won't really affect it and it works now.

github-actions[bot] commented 8 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 7 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.

github-actions[bot] commented 7 months ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (1)

tcontext

Previously acknowledged words that are now absent listheader mediatheme notlike npmrc PII Portainer prebuild taxonomyfield visualstudioextension vuejs 🫥
To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands ... in a clone of the [git@github.com:Lombiq/Open-Source-Orchard-Core-Extensions.git](https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions.git) repository on the `issue/OSOE-751` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/Lombiq/GitHub-Actions/dev/apply.pl' | perl - 'https://github.com/Lombiq/Open-Source-Orchard-Core-Extensions/actions/runs/7977451883/attempts/1' ```
Available :books: dictionaries could cover words not in the :blue_book: dictionary Dictionary | Entries | Covers | Uniquely -|-|-|- [cspell:typescript/dict/typescript.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/typescript/dict/typescript.txt)|1098|1|1| Consider adding them (in `.github/workflows/build-and-test.yml`) to `extra_dictionaries`: ``` yml cspell:typescript/dict/typescript.txt ``` To stop checking additional dictionaries, add (in `.github/workflows/build-and-test.yml`): ``` yml check_extra_dictionaries: '' ```
github-actions[bot] commented 7 months ago

This pull request has merge conflicts. Please resolve those before requesting a review.