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.

Sign up for GitHub

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

Jump to bottom

IDEMPIERE-5181 : Share process saved parameters #1525

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

IDEMPIERE-5181 : Share process saved parameters #1525

wants to merge 2 commits into from

Conversation

nmicoud
Copy link
Contributor

@nmicoud nmicoud commented Oct 12, 2022

https://idempiere.atlassian.net/browse/IDEMPIERE-5181

Pull Request Checklist

Checklist:

  • My code follows the code guidelines of this project
  • My code follows the best practices of this project
  • I have performed a self-review of my own code
  • My code is easy to understand and review.
  • In hard-to-understand areas, I have commented my code.
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • I have added unit tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have made corresponding changes to the documentation as follows:
    • New feature (non-breaking change which adds functionality): I have created the New Feature page in the project wiki explaining the functionality and how to use it. If relevant, I have committed sample data to the core seed to have usable examples in GardenWorld.
    • Breaking change (fix or feature that would cause existing functionality to not work as expected): I have documented the change in a clear way that everyone in the community can understand the impact of the change.
    • Improvement (improves and existing functionality): This documentation is needed if the improvement changes the way the user interacts with the system or the outcome of a process/task changes. If it is just, for instance, a performance improvement, documentation might not be needed.
  • The changed/added documentation is in the project wiki (not privately-hosted pdf files or links pointing to a company website) and is complete and self-explanatory.

@nmicoud nmicoud mentioned this pull request Oct 12, 2022
Closed
@nmicoud
Copy link
Contributor Author

nmicoud commented Oct 12, 2022

WIP - need review before proceeding, see comments in ticket

@nmicoud nmicoud marked this pull request as ready for review November 23, 2022 16:27
@nmicoud
Copy link
Contributor Author

nmicoud commented Dec 6, 2022

hi @CarlosRuiz-globalqss / @hengsin ,
Will you have time to have a quick look at this PR before release of v10 ?
Just to know if it worth to work on the subject in the next few days or if I wait for v11 (so, just need to know if it sounds like correct or not)
Thanks
Nicolas

hengsin
hengsin reviewed Apr 5, 2023
View reviewed changes
org.adempiere.base/src/org/compiere/model/MPInstance.java
.setClient_ID()
StringBuilder whereClause = new StringBuilder("AD_Process_ID = ? AND Name IS NOT NULL AND (")
.append(" (AD_Client_ID = 0 AND AD_Role_ID IS NULL AND AD_User_ID IS NULL)")
.append(" OR (AD_Client_ID = ? ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think we should include AD_Client_ID=0 here

hengsin
hengsin reviewed Apr 5, 2023
View reviewed changes
org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/AbstractProcessDialog.java
sp.setAD_Org_ID(0);
sp.setAD_Role_ID(-1);
sp.setAD_User_ID(-1);
sp.saveEx();
Copy link
Collaborator

Choose a reason for hiding this comment

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

is duplicated "Name" allows here ?

hengsin
hengsin reviewed Apr 5, 2023
View reviewed changes
org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/AbstractProcessDialog.java
MPInstance sp = savedParams.get(i);
sp.setAD_Org_ID(0);
sp.setAD_Role_ID(-1);
sp.setAD_User_ID(-1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps it would be nice if we can set the sharing scope, i.e for specific user, role or everyone.

@hengsin
Copy link
Collaborator

hengsin commented Apr 5, 2023

hi @nmicoud , there's conflict to resolve here. Since this is a new UI addition, I think it would be useful to get Carlos's help to setup a test server for it to get feedback from others.

@nmicoud
Copy link
Contributor Author

nmicoud commented Apr 5, 2023

hi @nmicoud , there's conflict to resolve here. Since this is a new UI addition, I think it would be useful to get Carlos's help to setup a test server for it to get feedback from others.

Hi,
I can try to resolve conflicts. I can't guarantee when, but hope to be able to do this before end of next week.

@nmicoud
Copy link
Contributor Author

nmicoud commented Apr 14, 2023

Sorry, I didn't find the time to work on this ; I'll be away next week and so hope that can be done by the end of April

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hengsin hengsin hengsin left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@nmicoud @hengsin