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

Fixing getFileName and layouts in module builder and studio. #3918

Closed

Conversation

daniel-samson
Copy link
Contributor

@daniel-samson daniel-samson commented Jul 19, 2017

Description

This change fixes some issue with module builder and studio. I have also added PHP doc to the files I have changed and updated the license headers in each class. I have only updated the access modifiers, where I believe it is safe to do so.

Issue 1:
When you create a new module in module builder, you are unable to select any of the layouts or edit fields.

Issue 2:
When you create a new module in module builder, the filter icons displays the incorrect icon and the text is blank.

Issue 3:
I have updated the classes which call getFileName method call so that they handle the new packageName variable.

Motivation and Context

  • Fixes some issues

How To Test This

  • Repair and Rebuild
  • Test each feature for
    • Module Builder
    • Studio

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

@Dillon-Brown Dillon-Brown added the Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member label Jul 19, 2017
@samus-aran samus-aran added Status:Needs Assessed Needs the core team to assess and removed Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member labels Jul 20, 2017
Copy link
Contributor

@gymad gymad left a comment

Choose a reason for hiding this comment

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

I approve it and I would to ask to in the future please can you make separated commit or PR for code cleanup only?

@pgorod
Copy link
Contributor

pgorod commented Jul 20, 2017

That an interesting question that @gymad raises - these PR's are harder to read and understand when so much code cleanup is in there too. It is difficult to see exactly what the PR is intending to change, and what are just formatting changes.

But we need to be careful with code cleanups because they also require testing. We've been bitten by this more than once - a tiny mistake in a change that was not intended to change the way the code works, but ends up breaking things (like that query that lost a space character in 7.9.0...).

So we would probably want to be able to do code reviews with separated fixes/code cleanups, but then test them together (if they affect the same files). I don't know if this is practical, or exactly how it be achieved...

@horus68
Copy link
Contributor

horus68 commented Jul 20, 2017

@gymad On code cleanup I made the same request for language files: all SuiteCRM code files should have a code cleanup version, as in full files edit without code changes and a new version should be released for that.
This is needed now because of the Coding standards approved and once every year for copyright statements.
This will avoid having to change each files when there is a need to change the code on that file. But then I found that the option was to only a file change when it was needed. This do not allow quality and time speed.

Code cleanup required to follow the Coding Standards https://suitecrm.com/wiki/index.php/Coding_Standards:

  • License header for every file (languages are now done)
  • Entry point if - die (languages are now done)
  • Copyright version - this needs to be done every January (languages was updated for 2017 on version 7.9.3)
  • Code Indentation normalization (not done!)
  • Use single quotes in language files + Arrays formatting (not done!)

I believe this would take some days for the same person but will avoid any further time lost to everyone else.
Can you please discuss this internally with SalesAgility?

@fernengel
Copy link

fernengel commented Aug 1, 2017

Hi!

Sorry I'm a bit late on commenting...
I originally added a comment to issue #4032 (layout editing does not work on Module Builder).

Unfortunately I don't know yet how to get the pull request to test your fix, but I corrected the DeployedMetaDataImplementation.php-file manually just to see if it works (added argument $packageName to getFileName()).

Therefore I might have missed other fixes of your pull request...but my question is:
Is the fix tested sufficiently so that both Module Builder AND Studio work?
Maybe I broke my SuiteCRM, but I'm having problems with editing the Layouts in "Studio".
Also, even though the Module Builder now loads the layout-editing-window again, it does not seem to work reliably. It replaces fields in the layout always with the field "Modified By Name".

So, don't panic :) Maybe I just broke my instance.
But anybody who tested the fix...have you made sure, Module Builder AND Studio are working and are actually usable?

Thanks a lot!

Best regards
David

@JBrace1990
Copy link

This has also broken all of our tabs and turns them into panels whenever the views are updated.

@Dillon-Brown Dillon-Brown added Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member and removed Status:Needs Assessed Needs the core team to assess labels Aug 14, 2017
@daniel-samson
Copy link
Contributor Author

see #4097

@Dillon-Brown Dillon-Brown removed the Status:Assessed PRs that have been tested and confirmed to resolve an issue by a core team member label Sep 6, 2017
@daniel-samson daniel-samson deleted the fix_mb_studio branch November 20, 2017 11:09
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.

None yet

8 participants