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

Return only pages from current site in api #2169

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Return only pages from current site in api #2169

merged 2 commits into from
Aug 20, 2021

Conversation

afdev82
Copy link
Contributor

@afdev82 afdev82 commented Aug 10, 2021

What is this pull request for?

If we search the internal page to be linked when we create a node in the menu, the results span all the websites.
This is for me not necessary and it gives troubles if many pages have the same name and paths.

Notable changes (remove if none)

This PR filter the pages to be the ones available only in the current alchemy site.

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: unrecognized cop Lint/DuplicateBranch found in .rubocop.yml, unrecogni...
Error: unrecognized cop Lint/DuplicateBranch found in .rubocop.yml, unrecognized cop Lint/DuplicateRegexpCharacterClassElement found in .rubocop.yml, unrecognized cop Lint/EmptyBlock found in .rubocop.yml, unrecognized cop Lint/EmptyClass found in .rubocop.yml, unrecognized cop Lint/NoReturnInBeginEndBlocks found in .rubocop.yml, unrecognized cop Lint/ToEnumArguments found in .rubocop.yml, unrecognized cop Lint/UnexpectedBlockArity found in .rubocop.yml, unrecognized cop Lint/UnmodifiedReduceAccumulator found in .rubocop.yml, unrecognized cop Style/ArgumentsForwarding found in .rubocop.yml, unrecognized cop Style/CollectionCompact found in .rubocop.yml, unrecognized cop Style/DocumentDynamicEvalDefinition found in .rubocop.yml, unrecognized cop Style/NegatedIfElseCondition found in .rubocop.yml, unrecognized cop Style/NilLambda found in .rubocop.yml, unrecognized cop Style/RedundantArgument found in .rubocop.yml, unrecognized cop Style/SwapValues found in .rubocop.yml

@tvdeyen
Copy link
Member

tvdeyen commented Aug 16, 2021

@afdev82 thanks, that makes sense. Would you mind to add a test that confirms this change?

@tvdeyen tvdeyen added this to the 6.0 milestone Aug 16, 2021
@afdev82
Copy link
Contributor Author

afdev82 commented Aug 16, 2021

Curious... I just found that: ace6ccb
the "feature" seems to be present in v6, but since I'm using the v5.2 I don't have it.
I consider it more a bug fix anyway, I would like to hear your opinion about that, could we include it in v5.2?
I'm currently waiting till the v6 is released to update Alchemy to it.

@tvdeyen
Copy link
Member

tvdeyen commented Aug 16, 2021

@afdev82 Ah, did recognize that this PR is for 5.2. Would you mind to cherry-pick ace6ccb instead?

@afdev82
Copy link
Contributor Author

afdev82 commented Aug 16, 2021

I found that it was changed by: 02d7e1d
should I cherry pick also that commit?

@tvdeyen
Copy link
Member

tvdeyen commented Aug 16, 2021 via email

As with the show action we do not want to return pages for other sites
except the current one.
CanCanCan does not respect any scope set before `accessible_by`.
We need to make sure the additional scopes get called afterwards.
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: unrecognized cop Lint/DuplicateBranch found in .rubocop.yml, unrecogni...
Error: unrecognized cop Lint/DuplicateBranch found in .rubocop.yml, unrecognized cop Lint/DuplicateRegexpCharacterClassElement found in .rubocop.yml, unrecognized cop Lint/EmptyBlock found in .rubocop.yml, unrecognized cop Lint/EmptyClass found in .rubocop.yml, unrecognized cop Lint/NoReturnInBeginEndBlocks found in .rubocop.yml, unrecognized cop Lint/ToEnumArguments found in .rubocop.yml, unrecognized cop Lint/UnexpectedBlockArity found in .rubocop.yml, unrecognized cop Lint/UnmodifiedReduceAccumulator found in .rubocop.yml, unrecognized cop Style/ArgumentsForwarding found in .rubocop.yml, unrecognized cop Style/CollectionCompact found in .rubocop.yml, unrecognized cop Style/DocumentDynamicEvalDefinition found in .rubocop.yml, unrecognized cop Style/NegatedIfElseCondition found in .rubocop.yml, unrecognized cop Style/NilLambda found in .rubocop.yml, unrecognized cop Style/RedundantArgument found in .rubocop.yml, unrecognized cop Style/SwapValues found in .rubocop.yml

@tvdeyen tvdeyen merged commit 504c5bf into AlchemyCMS:5.2-stable Aug 20, 2021
tvdeyen added a commit that referenced this pull request Sep 15, 2021
- Return only pages from current site in api [#2169](#2169) ([afdev82](https://github.com/afdev82))
- Improve cache key defaults for menus #2138 [#2160](#2160) ([oneiros](https://github.com/oneiros))
- generate picture thumbnails only for pictures with convertible format [#2130](#2130) ([afdev82](https://github.com/afdev82))
- Backport #2114 to v5.2 [#2116](#2116) ([afdev82](https://github.com/afdev82))
- Add webpacker tasks to Alchemy upgrader [#2115](#2115) ([dbwinger](https://github.com/dbwinger))
@dbwinger
Copy link
Contributor

dbwinger commented Nov 3, 2021

@tvdeyen @afdev82 I'm running into problems I think related to this. When I go to set a page on an EssenceLink content, the auto-complete input only shows pages for the site matching the host I'm currently on. That can work, but it's not ideal when managing multiple sites with different hosts. If one of my sites ("Test 1") is 'test1.com', and the other ("Test 1") is 'test2.com', that means if I'm editing a page on site "Test 2" at 'test1.com/admin/pages/1', the only pages I'm offered for links are ones on site "Test 1". It requires me signing into Alchemy Admin from the other domain "test2.com" in order to be able to set links. I think it should instead use the language of the page being edited in the case of EssenceLink contents. Am I missing something or is this a bug/missing feature?

@tvdeyen
Copy link
Member

tvdeyen commented Nov 3, 2021

@tvdeyen @afdev82 I'm running into problems I think related to this. When I go to set a page on an EssenceLink content, the auto-complete input only shows pages for the site matching the host I'm currently on. That can work, but it's not ideal when managing multiple sites with different hosts. If one of my sites ("Test 1") is 'test1.com', and the other ("Test 1") is 'test2.com', that means if I'm editing a page on site "Test 2" at 'test1.com/admin/pages/1', the only pages I'm offered for links are ones on site "Test 1". It requires me signing into Alchemy Admin from the other domain "test2.com" in order to be able to set links. I think it should instead use the language of the page being edited in the case of EssenceLink contents. Am I missing something or is this a bug/missing feature?

@dbwinger that sounds like a reasonable fix for that issue. We would need to pass the current pages language to the link overlay. Doable for sure. Mind to open a ticket and even try to work on a solution?

@dbwinger
Copy link
Contributor

dbwinger commented Nov 3, 2021

@tvdeyen Sure I'll open a ticket, and I hope to get some time to work on it myself.

@afdev82 afdev82 deleted the filter_current_site_pages branch December 30, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants