search

Raingsey-Capricorn / amf-registration

Acme Movie Fanatics (AMF) : Sample Project
0 stars 0 forks source link

Comments and Issues #1

Open mauricesepe opened 3 years ago

mauricesepe commented 3 years ago

Please follow naming conventions

Sample https://github.com/liferay/liferay-portal-ee/tree/fix-pack-dxp-17-7110/modules/apps/blogs

mauricesepe commented 3 years ago

Can you explain why we need this?

mauricesepe commented 3 years ago

I'm not sure about the exercise requirements, but is it really necessary to create separate entities for this project (event log, user, etc) Can we not use OOTB models?

mauricesepe commented 3 years ago

For every user registered, you are creating a different site. Is this requirement correct?

https://github.com/Raingsey-Capricorn/amf-registration/blob/ca44e2beee0f46c7666c3b71e0254902e91d7eee/modules/amf.registration.core/amf.registration.core-service/src/main/java/com/amf/registration/service/impl/AMFUserLocalServiceImpl.java#L384-L409

Raingsey-Capricorn commented 3 years ago

I'm not sure about the exercise requirements, but is it really necessary to create a separate model for the AMFUser? Can we not use OOTB User object?

Hi, at first time I don't know what OOTB, how to use it, because this pretty difficult to make it works let alone customizing the Registration process. I tried checking how the service-builder works, what fields to use, what implementation I need to make it works. This is not the cleanest implementation, I decided to create this object AMFUser simply to copy some attribute from the User itself, to make it works first.

There are 2 ways to customize the registration, 1- apply OOTB, like you mentioned, that I couldn't make it work at the first place 2- create additional object to hold some field needed to save the user, which was the only way I could think of at the first time.

mauricesepe commented 3 years ago

You've used dynamic query for search and I think that's fine. If you want to integrate your new entities and use elastic search, you can use indexers / contributors.

https://help.liferay.com/hc/en-us/articles/360032260632-Indexing-Model-Entities

mauricesepe commented 3 years ago

I think this should call Service instead of LocalService. Might need to perform permissions checks right? Also better to use @Reference tag and call the service directly instead of using Util.

https://github.com/Raingsey-Capricorn/amf-registration/blob/ca44e2beee0f46c7666c3b71e0254902e91d7eee/modules/amf.registration.portlet/src/main/java/com/amf/registration/portlet/portlet/command/action/AMFRegisterMVCActionCommand.java#L52

Raingsey-Capricorn commented 3 years ago

For every user registered, you are creating a different site. Is this requirement correct?

https://github.com/Raingsey-Capricorn/amf-registration/blob/ca44e2beee0f46c7666c3b71e0254902e91d7eee/modules/amf.registration.core/amf.registration.core-service/src/main/java/com/amf/registration/service/impl/AMFUserLocalServiceImpl.java#L384-L409

For this implementation, I am not so sure, actually, because there's no sufficient document to support my research, and what I got was trying to test step by step, line by line and understand all the way, and that was so frustrated to do. So some parts were left incorrect just to make the saving works, at first

Raingsey-Capricorn commented 3 years ago

You've used dynamic query for search and I think that's fine. If you want to integrate your new entities and use elastic search, you can use indexers / contributors.

https://help.liferay.com/hc/en-us/articles/360032260632-Indexing-Model-Entities

HI, for the search, I used this mainly base on my knowledge for search using the query in and didn't think of the way to apply elastic search just yet, what's more I didn't think of apply the elastic search since I was unclear how to make it works, as I couldn't find developing support document to make it works as I needed at the time

Raingsey-Capricorn commented 3 years ago

Please follow naming conventions

  • amf-registration
    • amf-registration-web
    • amf-registration-impl
    • amf-registration-api

Sample https://github.com/liferay/liferay-portal-ee/tree/fix-pack-dxp-17-7110/modules/apps/blogs

Hi, yeah, it was IntelliJ plug-in module structure, I changed it and I broke once, so I decided to keep it that way. I will update this module structure-naming conventions later. Thanks

Raingsey-Capricorn commented 3 years ago

I think this should call Service instead of LocalService. Might need to perform permissions checks right? Also better to use @reference tag and call the service directly instead of using Util.

https://github.com/Raingsey-Capricorn/amf-registration/blob/ca44e2beee0f46c7666c3b71e0254902e91d7eee/modules/amf.registration.portlet/src/main/java/com/amf/registration/portlet/portlet/command/action/AMFRegisterMVCActionCommand.java#L52

Yeah, I agree with that, at first time, I didn't understand about the service-builder code generator works, so I tried not to break it, just keep the default as much as possible. I will update this part for the next commits. Thanks

Raingsey-Capricorn commented 3 years ago

Can you explain why we need this?

Hi, for these two fields, at first time, I didn't know about the way the service-builder works and I tried to add additional fields to store data in the database, try not to mess up with the built-in User entity and the _user table, that's why I added these two fields, later on I will refactor and clean up the unnecessary fields. Thanks