8000 fix: field replace by blaggacao · Pull Request #21419 · frappe/frappe · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: field replace #21419

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
Merged
merged 4 commits into from
Jul 4, 2023
Merged

fix: field replace #21419

merged 4 commits into from
Jul 4, 2023

Conversation

blaggacao
Copy link
Contributor
@blaggacao blaggacao commented Jun 18, 2023 •
edited
Loading
  • fix: section fields_dict
  • fix: replace_field

Prior to this PR, replace_field did simply not work. Because:

  • splice needs an (integer) index not an element
  • also section field management must be taken into account for all sections

This PR uses the correct integer for splicing and also manages sections, columns and tabs
according to the original field that is being replaced so that the field remains at the same
location in the layout.

works blissfully in tandem with #21322 :

	setup(frm) {
		df = frm.get_docfield("map");
		df["fieldtype"] = "GeolocationVRP";
		frm.layout.replace_field("map", df);
	},
	refresh(frm) {
		fld = frm.fields_dict["map"];
		fld.refresh(); // not sure why necessary, may be rather a bug in ControlGeolocation
	}

And a custom controller:

frappe.ui.form.ControlGeolocationVRP = class ControlGeolocationVRP extends (
	frappe.ui.form.ControlGeolocation
) {
	point_to_layer(geoJsonPoint, latlng) {
		// Custom rules for how geojson data is rendered on the map
		if (geoJsonPoint.properties.point_type == "circle") {
			return L.circle(latlng, { radius: geoJsonPoint.properties.radius });
		} else if (geoJsonPoint.properties.point_type == "circlemarker") {
			return L.circleMarker(latlng, {
				radius: geoJsonPoint.properties.radius,
			});
		} else {
			// return L.marker(latlng);
			const homeIcon = L.divIcon({ className: "fa fa-home --blue" });
			return L.marker(latlng, { icon: homeIcon });
		}
	}
	setStyle(geoJsonFeature) {
		if (geoJsonFeature.geometry.type === "LineString") {
			return {
				color: geoJsonFeature.properties.stroke,
				weight: geoJsonFeature.properties["stroke-width"],
			};
		}
		return {};
	}
};

@blaggacao blaggacao requested review from a team and shariquerik and removed request for a team June 18, 2023 01:50
@codecov
Copy link
codecov bot commented Jun 18, 2023 •
edited
Loading

Codecov Report

Merging #21419 (4cc1fde) into develop (00e5dab) will decrease coverage by 0.02%.
The diff coverage is 12.50%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #21419      +/-   ##
===========================================
- Coverage    63.86%   63.85%   -0.02%     
===========================================
  Files          765      765              
  Lines        69353    69364      +11     
  Branches      6271     6272       +1     
===========================================
- Hits         44291    44289       -2     
+ Misses       21487    21485       -2     
- Partials      3575     3590      +15
Flag Coverage Δ
server-ui 29.93% <ø> (-0.01%) ⬇️
ui-tests 51.45% <12.50%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@blaggacao blaggacao force-pushed the fix-field-replace branch from 3212710 to 5f76e97 Compare June 18, 2023 02:09
@blaggacao blaggacao closed this Jun 18, 2023
@blaggacao
Copy link
Contributor Author

something's still missing here.

@blaggacao blaggacao reopened this Jun 18, 2023
@blaggacao
Copy link
Contributor Author
blaggacao commented Jun 18, 2023 •
edited
Loading

This was the missing bit: 7592cbe ecec197

@blaggacao blaggacao mentioned this pull request Jun 18, 2023
Merged
@blaggacao blaggacao force-pushed the fix-field-replace branch from 7592cbe to bafadf0 Compare June 18, 2023 22:29
@blaggacao blaggacao force-pushed the fix-field-replace branch from bafadf0 to ecec197 Compare June 20, 2023 17:44
@blaggacao
Copy link
Contributor Author

@shariquerik I just rebased on latest develop. So this branch now represents the minimal change set.

barredterra
barredterra reviewed Jun 22, 2023
View reviewed changes
Copy link
Collaborator
@barredterra barredterra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

From the PR description, it's not clear to me, what you are trying to achieve with this PR.

Some nitpicks:

  • The PR title should follow the conventional commit style
  • The variable names (fieldobj, oldfieldobj, olfldobj) are hard to read and make sense of

@blaggacao
Copy link
Contributor Author
blaggacao commented Jun 22, 2023 •
edited
Loading

@barredterra this is a fix for an issue not covered by a test case. I'd usually have supplied a failing one, but I'm not yet proficient enough, so bear with me 😄

I'll double check on CC commit style. I thought I had checked with pre-commit. Double checking...

Ah! PR Title! Yeah, saw a reference to some pecularities somewhere, thanks for reminding me!

Variable names: ack! 👀

@blaggacao
Copy link
Contributor Author
blaggacao commented Jun 22, 2023 •
edited
Loading

How could I unit-test DOM initialization? I'm not sure how much of a public API the implementation exposes.

While looking at it, it seemed all a bit private api, so not sure how I'd approach a testcase without being sent on an integration test tangent. But maybe there's a way?

@blaggacao blaggacao changed the title fix field replace fix: field replace Jun 24, 2023
@blaggacao blaggacao requested a review from barredterra June 24, 2023 18:17
@blaggacao blaggacao force-pushed the fix-field-replace branch 3 times, most recently from e46307e to 5b24bce Compare June 26, 2023 02:48
blaggacao added 2 commits July 1, 2023 12:10
@blaggacao
fix: replace_field
8000 c415e27 
splice needs an (integer) index not an element

also section field management must be taken into account for all sections
@blaggacao
fix: replace field replacing correct dom element
60231c7
@blaggacao blaggacao force-pushed the fix-field-replace branch from 5b24bce to 60231c7 Compare July 1, 2023 17:11
@blaggacao
Copy link
Contributor Author

@shariquerik @barredterra So what's the game plan for this PR?

  • Rebased on develop
  • Attended all reviewer feedback (or so I hope 😄)

Imo, this is ready to merge.

shariquerik
shariquerik requested changes Jul 3, 2023
View reviewed changes
@blaggacao
Copy link
Contributor Author

@shariquerik Thanks for the fix! And thanks for motion to merge!

@shariquerik shariquerik merged commit 3549fe7 into frappe:develop Jul 4, 2023
@shariquerik shariquerik added the backport version-14-hotfix backport to version 14 label Jul 4, 2023
@mergify mergify bot mentioned this pull request Jul 4, 2023
Merged
@shariquerik
Copy link
Member

@blaggacao Can you resolve the conflicts in backported PR?

shariquerik added a commit that referenced this pull request Jul 10, 2023
@shariquerik
Merge pull request #21583 from frappe/mergify/bp/version-14-hotfix/pr…
< 830B code> c765f23 
…-21419

fix: field replace (backport #21419)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
@blaggacao blaggacao deleted the fix-field-replace branch December 16, 2023 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shariquerik shariquerik shariquerik approved these changes

@barredterra barredterra Awaiting requested review from barredterra

Assignees

@shariquerik shariquerik

Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blaggacao @shariquerik @barredterra
0