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

feat: Support Many to Many relationships #139

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

candidosales
Copy link

@candidosales candidosales commented Oct 10, 2023

What does it do?

  • getAllowedFields showing fields with relationship ManyToMany;
    • Included mappedByand inversedBy;
  • Improved regex in validatePattern to support fields with array. Ex: [categories[0].slug];
  • Generate sitemap correctly;

Why is it needed?

You can select multiple relationships and their attributes using this feature.

How to test it?

CleanShot.2023-10-09.at.23.11.44.mp4

Related issue(s)/PR(s)

#78

Let us know if this is related to any issue/pull request

@candidosales candidosales changed the title [WIP] feat: Support to Many Relationshipg [WIP] feat: Support to Many Relationships Oct 10, 2023
@candidosales candidosales changed the title [WIP] feat: Support to Many Relationships [WIP] feat: Support Many to Many relationships Oct 10, 2023
server/services/pattern.js Outdated Show resolved Hide resolved
server/services/pattern.js Outdated Show resolved Hide resolved
@candidosales candidosales changed the title [WIP] feat: Support Many to Many relationships feat: Support Many to Many relationships Oct 10, 2023
@candidosales
Copy link
Author

@boazpoolman , could you review it? 😃

Copy link
Member

@boazpoolman boazpoolman left a comment

Choose a reason for hiding this comment

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

Just basing this question off of your screenrecording;

Is it possible to select different indexes in the array? For example [relation[1].field]

@candidosales
Copy link
Author

@boazpoolman , it isn't possible. Because the validation pattern only accepts the exact allowed field names, we can improve it in the next PR.

CleanShot 2023-10-10 at 06 41 02@2x

@boazpoolman
Copy link
Member

I would suggest solving that in this PR.
It's an essential feature when we introduce the relation array in url patterns.

@candidosales
Copy link
Author

candidosales commented Oct 10, 2023

@boazpoolman , it's fixed in my latest commit 😃 . I'm fixing the lint issues

@candidosales
Copy link
Author

@boazpoolman , it's ready to review it

@boazpoolman
Copy link
Member

Great. I'll review as soon as I've got the time!

Copy link
Member

@boazpoolman boazpoolman left a comment

Choose a reason for hiding this comment

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

The eslint job is still failing.

Also it's very hard to review this PR because you've auto-fixed the files based on some prettier config that does not originate from the plugin.

It would be amazing if you could revert all those changes so that the diff of this PR is simply the code needed to implement the Many to Many relation feature.

@candidosales
Copy link
Author

@boazpoolman It's ready to review.

The changes are:

  • getAllowedFieldsadded support to showing fields with relationship ManyToMany;
  • getFieldsFromPattern updated the Regex to support array fields;
  • resolvePatternadded support to interact with array values and your specific indexes;

Now, it's easier to review it.

@candidosales
Copy link
Author

@boazpoolman , could you review it?

Copy link
Member

@boazpoolman boazpoolman left a comment

Choose a reason for hiding this comment

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

For starters:
Thanks @candidosales for submitting this PR.

I've reviewed it and I'm having some issues.

  1. If I use a toOne relation I see the dynamic value I can insert into the pattern (e.g. [category.id]). But if I add a toMany relation I don't see the field in the pattern description.
  2. If I add the dynamic value anyways (e.g. [category[0].id]) and I try to save I get an internal server error.

Please see my video in which you can witness these two issues:

sitemap.mp4

@candidosales
Copy link
Author

@boazpoolman , great! Thanks for reviewing it!

I'll take a look.

@boazpoolman
Copy link
Member

Hi @candidosales

Have you been able to look at my feedback?

@candidosales
Copy link
Author

Hi @boazpoolman , I didn't have time yet to improve it. Sorry about that. Many things are happening.

@Convolva
Copy link

Convolva commented Jul 1, 2024

Hi, I think this is a much needed functionality. Is there any timeline for this PR @candidosales ?

Usually collection types like 'Blogs' and 'categories' tend to have a many to many relationship with the URL structure changing based on the category slug and the blog slug. So this feature could be really helpful for those kinds of setup.

cc: @boazpoolman

@candidosales
Copy link
Author

Finally, I have time to take a look. @boazpoolman, could you review it?

@boazpoolman boazpoolman added the Needs review A PR that needs to be reviewed label Jul 18, 2024
Copy link
Collaborator

@MSACC MSACC left a comment

Choose a reason for hiding this comment

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

We have tested the new code you provided us and we still get an internal server error. We hope you can fix. We get the error error: Cannot read properties of null (reading '0') TypeError: Cannot read properties of null (reading '0') as previewed in the video @boazpoolman previously sent.

Greetings, Mathijs from PluginPal

@candidosales
Copy link
Author

@MSACC, could you share a video showing the issue? I would like to replicate

@boazpoolman boazpoolman added Needs work A PR that needs to be modified and removed Needs review A PR that needs to be reviewed labels Jul 26, 2024
@boazpoolman
Copy link
Member

Mathijs experienced the same issues I did when I tested it.
I posted a video of it back then, see my comment.

@n1kalai
Copy link

n1kalai commented Aug 6, 2024

great feature guys, can't wait

@renatosilvarosa
Copy link

@candidosales, this is a useful feature, and I also have this use case in a project I'm working on. Would you like some help finishing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs work A PR that needs to be modified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants