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

Feature/maniphest edit 2.0 #55

Closed
wants to merge 20 commits into from

Conversation

D0wn10ad
Copy link

Fixes: Version bump to 0.3.0 in code, added features to enable more parameters to be set

Copy link
Contributor

@Grokzen Grokzen left a comment

Choose a reason for hiding this comment

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

In general these changes look very nice, they need a bit polishing and linting mostly. We have not done in-depth testing yet and once we do that we can give a bit more feedback on the feature content itself

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
phabfive/cli.py Show resolved Hide resolved
phabfive/constants.py Show resolved Hide resolved
phabfive/maniphest.py Outdated Show resolved Hide resolved
Comment on lines +258 to +267
if ("owner" in ticket_data):
rendered_data[r-1]["owner"] = ticket_data["owner"]
if ("priority" in ticket_data):
rendered_data[r-1]["priority"] = ticket_data["priority"]
if ("projects" in ticket_data):
rendered_data[r-1]["projects"] = ticket_data["projects"]
if ("subscribers" in ticket_data):
rendered_data[r-1]["subscribers"] = ticket_data["subscribers"]
if ("parents" in ticket_data):
rendered_data[r-1]["parents"] = ticket_data["parents"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless it is needed for any of the logic it checks, parenthesis should be avoided as they are not required or needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, might be unpushed changes but parenthesis still remains

phabfive/maniphest.py Show resolved Hide resolved
phabfive/maniphest.py Show resolved Hide resolved
phabfive/maniphest.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Grokzen Grokzen left a comment

Choose a reason for hiding this comment

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

@D0wn10ad A additional changes to be included. After they are sorted we are happy with merging this.

One thing that is missing is a few updates to the CHANGELOG.md file that would be included in the release after this is merged

Comment on lines +63 to +73
r = self.phab.project.search(constraints = {"name": ""})
length = len(r.data)
a = r['cursor']['after']
projects_query["data"].extend(r.data)

# Added for loop to handle phab instances with more than 100 project/tags
while (length >= 100 and a is not None):
r = self.phab.project.search(constraints = {"name": ""}, after=a)
length = len(r.data)
a = r['cursor']['after']
projects_query["data"].extend(r.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still single variable names here that we do not want in, at minimum variables should be 3 letters or more. Please update this through out the entire PR

afterCursor = r['cursor']['after']
users_query["data"].extend(r.data)

while (length >= 100 and afterCursor is not None):
Copy link
Contributor

Choose a reason for hiding this comment

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

So this might not need to be done in this PR yet but one idea for this kind of hack that seems to be needed all over in general, we could either do a simple helper function that wraps the cursor logic with a yield and a few more dynamic stuff for what endpoint to search, but then that code could be reused more efficiently.

In the long run i think we might want to go to the more Django Model based solution where things are wrapped within the logic of the models where we would sort this kind of problem in there and the end-user up here would never know. This however is for a future version and not within this PR


###FIXME: Add parent task
for index, ticket_data in enumerate(data["tickets"]):
if ("parents" not in ticket_data):
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove parenthesis here, they do not belong in if-case


if len(constraints["ids"]) == len(r.data):
for p in r.data:
taskid = "T" + str(p["id"])
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a f-string and not old style string concat

for p in r.data:
taskid = "T"+str(p["id"])
task_id_to_phid_mapping[taskid] = p["phid"]
print("adding",parent," ", p["phid"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Either you missed to push or forgot to change this, please check this one out again

Comment on lines +258 to +267
if ("owner" in ticket_data):
rendered_data[r-1]["owner"] = ticket_data["owner"]
if ("priority" in ticket_data):
rendered_data[r-1]["priority"] = ticket_data["priority"]
if ("projects" in ticket_data):
rendered_data[r-1]["projects"] = ticket_data["projects"]
if ("subscribers" in ticket_data):
rendered_data[r-1]["subscribers"] = ticket_data["subscribers"]
if ("parents" in ticket_data):
rendered_data[r-1]["parents"] = ticket_data["parents"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, might be unpushed changes but parenthesis still remains

@Grokzen
Copy link
Contributor

Grokzen commented Aug 15, 2023

@D0wn10ad Any updates to this? we would like to get this in as we are soon pushing an update to the bulk import feature and would like this included before making next release 0.4.0

Grokzen added a commit that referenced this pull request Aug 23, 2023
…r updates in this PR

Updated readme with windows specific instructions
Improved the .search functionality as it had a bug where it would not parse more then 1 page of data due to
cursor not being implemented. Now there is a helper function to wrap searches so it will query all data for
any phabricator module.
Added a better output report when tickets is created
General code cleanup and tiny fixes
@Grokzen
Copy link
Contributor

Grokzen commented Aug 23, 2023

@D0wn10ad We have been developing a new v2.1 of the bulk importer with much more added functionality in #56 and i have blended in your changes from this PR where possible into my PR to merge our two new functionality so it will in the end be able to perform the same things you added and more.

I tried to base the work off your's here but it had diverged to far off from the first batch of changes we made in the other PR and your's was not completed so i only copied what i could over manually.

If you make any further changes or patches, please do them on the other PR:s content as-is now before we merge, or later when it is back in the master branch. This PR will be mentioned in the changelog so it won't be forgotten

@Grokzen Grokzen closed this Aug 23, 2023
Grokzen added a commit that referenced this pull request Nov 20, 2023
…r updates in this PR

Updated readme with windows specific instructions
Improved the .search functionality as it had a bug where it would not parse more then 1 page of data due to
cursor not being implemented. Now there is a helper function to wrap searches so it will query all data for
any phabricator module.
Added a better output report when tickets is created
General code cleanup and tiny fixes
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.

2 participants