Use display name fallback if blank display name result #6342
Conversation
e124f2c to
7b8f345
Compare
|
As discussed privately, my concern is that this introduces the inconsistency of potentially showing the value of different model attributes in the same column. So, in case it is intended that the value of the chosen display attribute is empty, then this would be a strict improvement. However, in the case where this is an unintended data problem, the current behaviour would make it easier to identify and fix, whereas the new behaviour would make a model whose name is So, although I don't currently have a great idea, I'd like us to at least apply some visual hint in the case where this "extra fallback" has been applied. I'll try to think about something more concrete. |
|
I think a visual hint is a great idea. Some other suggestions could be to also log a warning and/or use a configuration to opt-in to this change. |
|
Perhaps |
|
I don't think there's a "correct" solution involving changing only the wording of the name, because users should be free to name models whatever they like without causing conflicts with this, including the I mean, we could probably use some virtually impossible name like |
|
A quick fix could also be to replace blank values by a non-breaking space. This would, at least, make the link appear and be clickable. This would help show visually that the association is present but the name is empty. E.g.:
This would not be ideal since the link is very small, though. |
|
Well, that's definitely better than what we have now, and it doesn't bring any ambiguity. Thoughts @javierjulio? |
|
Thanks for the suggestion! Hmm I don't think so because by implementing that then we are actually giving a present value of Just to reiterate, there are two key items here. One is that the display name fallback is a good default overall but is not used in all cases. Second is that by just adding a column on a model, that does not require a value but just so happens to be in the |
7b8f345 to
efffbc0
Compare
|
@javierjulio Sorry for dropping the ball on this, I'll have another look as soon as possible. |
|
@deivid-rodriguez haha I was just updating to keep it current with CI/lint changes since then, so no worries, thanks! |
55d04e7 to
214cbc3
Compare
214cbc3 to
e3956c0
Compare
e3956c0 to
efcd5e7
Compare
efcd5e7 to
6a2d841
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #6342 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 196 196
Lines 4939 4940 +1
=======================================
+ Hits 4896 4897 +1
Misses 43 43
☔ View full report in Codecov by Sentry. |
This addresses an issue where if a model has a method that is one in the list of configured display_name_methods but it returns a blank value, then the display name will always come back blank. The display name fallback was only used if a model did not have one of the configured display names but it would be helpful to always rely on the display name fallback even if there is no return value otherwise this will cause side effects like this test case in its original form shown, the link had no value and so it wouldn't even appear to be clickable in a browser.
This addresses an issue where if a model has a method that is one in the list of configured display_name_methods but it returns a blank value, then the display name will always come back blank. The display name fallback was only used if a model did not have one of the configured display names but it would be helpful to always rely on the display name fallback even if there is no return value otherwise this will cause side effects like this test case in its original form shown, the link had no value and so it wouldn't even appear to be clickable in a browser.
dc8dbb8 to
aa5ed1d
Compare
The
display_namehelper only uses the display name fallback if no matching method is defined on a model from the configureddisplay_name_methodslist. The output of the display name fallback is the model human name and the primary key. So with a givenUsermodel and a numeric primary key, you would getUser #1as an example output.If the model implements a method from that
display_name_methodslist but it returns a blank value, this has a side effect where an association link is generated without text so it's not seen or clickable. I think it would help to use our display name fallback in this specific case since it is a good default, rather than be all or nothing. Thedisplay_name_methodslist is a global configuration that cannot be modified per resource.When working on this change, the test case I modified demonstrated the exact behavior I ran into. The anchor (link) HTML is generated but without a value so a user wouldn't see anything in the browser. While the test shows this is expected behavior, I think this would be an improvement for this specific scenario.
This change would not have any effect on cases where there is a custom display name because as long as it returns a present value, it will always be used. This is a small improvement to rely on a good default, our display name fallback, when there is no present value.
Let me know your thoughts and any suggestions for changes here.