Support external link in search promotions #10852
Conversation
Manage this branch in SquashTest this branch here: https://topdevprosmain-0bc4l.squash.io |
|
Thanks for fixing the CI addition. Could you review the formatting CI failure, should be pretty minor.
Additionally, could you merge the migration files into one file. This can be done by deleting the migration files and then running the make migrations task again. Then commit all the changes together, deleting two, adding one. |
There was a problem hiding this comment.
Thank you for giving this a go @TopDevPros! Here is feedback based on the code only. In short I’d recommend:
- Removing all the comments that don’t directly explain the "why" of the code. If it looks like boilerplate comments just restating identifier names (class, method, function, variable), we don’t need it.
- Making link text mandatory
- Using the term "text" for the link text rather than "title".
| Attributes | ||
|
|
||
| - query_string (forms.CharField): Query text to match. | ||
| """ |
There was a problem hiding this comment.
I find those docstrings a bit distracting, and in this instance I can’t see any information in the comment that’s not already clear from the code. If this is for existing models where nothing changes, could we either not do this or do this in a separate PR? What’s the purpose here?
Similarly for method-level docstrings, they don’t seem to add much so I’d recommend removing them all (or if we think some of them are valuable, only add them to existing code in a separate PR).
There was a problem hiding this comment.
@TopDevPros this concern is still not addressed, adding docstrings to unrelated code changes is very distracting from this PR and we cannot merge until these are removed.
I understand your desire to 'clean things' or 'make things consistent', I too struggle with that.
However, our goal is a single (or small set) of commits that make one self-contained change. Changing these docstrings here is not required for this feature.
| REQUIRED_FIELDS_MISSING = _( | ||
| "You must recommend a related page OR an external link." | ||
| ) | ||
| TOO_MANY_FIELDS = _("Please only select a Page OR enter an external link.") |
There was a problem hiding this comment.
I only see those two used in a single place in the code. If there is no plan to use the message elsewhere, I think the code would be simpler to understand with the messages inlined?
| Args | ||
|
|
||
| - form (forms.Form): Form to add to form set | ||
| """ |
There was a problem hiding this comment.
Would suggest removing this. Since add_fields is part of the formset API there’s little value in documenting this here.
There was a problem hiding this comment.
As per this comment, please remove this docstring addition.
| """ | ||
| Search pick must have at least one recommended page or link to be valid. | ||
| Check there is at least one non-deleted form with the required field. | ||
| """ |
There was a problem hiding this comment.
This diff would be simpler to read if we only changed the message rather than also updated the comment style.
There was a problem hiding this comment.
I personally think it makes sense to update this to docstring while changing, this will be fine.
| {% if search_promotion.external_link_title %} | ||
| <h2>{{ search_promotion.external_link_title }}</h2> | ||
| {% else %} | ||
| <h2>{{ search_promotion.external_link_url }}</h2> |
There was a problem hiding this comment.
Here and in other parts of the code – I would say if external_link_url is in use then the link text should be required. URLs as link text make for a bad user experience, we can have content authors enter a real label.
| ) | ||
| sort_order = models.IntegerField(null=True, blank=True, editable=False) | ||
|
|
||
| external_link_url = models.URLField(_("URL"), blank=True) |
There was a problem hiding this comment.
| external_link_url = models.URLField(_("URL"), blank=True) | |
| external_link_url = models.URLField(_("external link URL"), blank=True) |
If we think a descriptive field name is warranted in the code, it’s probably an indication we also should have a descriptive label in the UI?
There was a problem hiding this comment.
This change has not been applied. I will just suggest we use title case though.
| external_link_url = models.URLField(_("URL"), blank=True) | |
| external_link_url = models.URLField(_("External link URL"), blank=True) |
There was a problem hiding this comment.
Sorry but this still has not changed to the full label.
| external_link_title = models.CharField( | ||
| _("Title"), max_length=32, blank=True, null=True | ||
| ) |
There was a problem hiding this comment.
I’d prefer we used a more descriptive label here as well. I’d suggest word "text" rather than "title" for link text as that seems clearer to me.
| external_link_title = models.CharField( | |
| _("Title"), max_length=32, blank=True, null=True | |
| ) | |
| external_link_text = models.CharField(max_length=32, blank=True, null=True) |
There was a problem hiding this comment.
Agree, this change still needs to be made.
There was a problem hiding this comment.
Can this please change to the suggested field name?
There was a problem hiding this comment.
"external_link_title" seems like a more appropriate name than "external_link_text" for 2 reasons: 1) the field is used in the "title" property when the search promotion is an external link instead of a page; 2) it is a char field, not a text field. An enhancement someone might like to make is adding a button the lets wagtail get the title for the URL the user entered and automatically fill in the field.
There was a problem hiding this comment.
Personally I agree with Thibaud's original suggestion, here's my reasoning.
- We already call this similar think 'text' when shown an external link in the link chooser modal.
- Using the term title could confuse the user into thinking this is a link title (as in the html attribute).
- If we ever want to add support for link title (unlikely but possible), we now need a migration.
- It's possible we would add support for link target though, which IS an attribute, hence we would have title (not the attribute title but the link text/content) and target (which would be the attribute).
- Having this a bit more generic leaves it up to implementors how they want to present this in their own search results list.
There was a problem hiding this comment.
Assuming 2 people's opinions make a consensus, we've made the change from "external_link_title" to "external_link_text". It might be a good idea for the wagtail team to clarify what they consider a consensus.
|
Thank you @thibaudcolas for your feedback. We have adjusted the code so the external title is required if the external URL is entered. Of course, we've added a test to verify that it works as desired. We will await some consensus from the core team on what to call the variables as @lb suggested the current names in both the code and on the form. Using "title" instead of "text" makes it easier to see that the external link title is a counterpart to the Page's title. Thank you for pointing out that we only used the constants that reported validation errors in the forms. The tests now use the form's constants. Wagtail's "Python coding guidelines" says to use PEP-8. PEP-8 says, "Write docstrings for all public modules, functions, classes, and methods." It's worth noting that there are very good reasons for a coding style standard, even though everyone disagrees with different parts of it. Again, we're happy to conform to the core team's preferences. Let us know if you'd prefer all docstrings removed or just the specific ones you commented on. It might be time for a review of Wagtail's "Python coding guidelines" and note exceptions (e.g., no docstrings, or only docstrings when ...). Thanks again, @thibaudcolas. We look forward to your feedback on the UI and decisions about variable names and docstrings. |
|
@TopDevPros I've started the CI. Just a note - my username is |
Sorry for the confusion, @lb -- I have edited the entry to use the correct username. |
|
We submitted a fix for Issue #10725. The requested functionality is done. The database initialization bug is fixed. The tests pass. We received a few reviews with suggestions. We have implemented the suggestions that don't violate PEP8 (Wagtail's stated standard]). What can we do now to move this PR along? |
There was a problem hiding this comment.
@TopDevPros thank you for continuing on this, there are still a bunch of items from @thibaudcolas 's review that were not yet implemented and I have gone through the whole PR to give specific feedback on all parts (except tests).
We really appreciate you taking the time to pursue this, I know the desire to 'fix up' other parts of the code (like naming variables or docstrings) but we cannot stress enough - please remove all of that unless explicitly related to this single feature.
Adding all the other changes means this PR cannot be properly reviewed and will not be able to be merged.
I recommend you create a new branch with a backup of what you have so far, this way if you want you can come back later and add docstrings.
I know there are Python practices for this to be added to every single method and document every single attribute but that is not the code style of this project and also is not the goal of your PR.
This PR is awesome and it will be an incredible feature to have, it just needs to be more focused and fix up a few of the UI naming things + the migration noise.
Thank you again, please review each item and ensure each is addressed and then add ac comment that it's ready for another review.
| Attributes | ||
|
|
||
| - query_string (forms.CharField): Query text to match. | ||
| """ |
There was a problem hiding this comment.
@TopDevPros this concern is still not addressed, adding docstrings to unrelated code changes is very distracting from this PR and we cannot merge until these are removed.
I understand your desire to 'clean things' or 'make things consistent', I too struggle with that.
However, our goal is a single (or small set) of commits that make one self-contained change. Changing these docstrings here is not required for this feature.
| """ | ||
| Form for search promotions | ||
|
|
||
| Attributes | ||
|
|
||
| - sort_order (forms.IntegerField): Order in sequence | ||
| """ | ||
|
|
There was a problem hiding this comment.
| """ | |
| Form for search promotions | |
| Attributes | |
| - sort_order (forms.IntegerField): Order in sequence | |
| """ |
This can be removed, it add not useful context.
| """ | ||
| Form set for search promotions | ||
|
|
||
| Attributes | ||
|
|
||
| - minimum_forms (int): Number of forms required for this set. | ||
| """ | ||
|
|
||
| # declare the constants here so we can use them in tests | ||
| # if the text changes, we don't need to update the tests |
There was a problem hiding this comment.
| """ | |
| Form set for search promotions | |
| Attributes | |
| - minimum_forms (int): Number of forms required for this set. | |
| """ | |
| # declare the constants here so we can use them in tests | |
| # if the text changes, we don't need to update the tests |
This can be removed, it add not useful context.
Additionally, a comment that says 'keep for tests' is not useful for anyone.
In fact, I would say it's better to duplicate the text in the tests, this is the pattern used throughout the entire project and it creates a cross-check for changes such as if things are removed.
Finally, please reset these to the current lower_snake_case convention, there is no need for these to change to class attributes, best to keep this code as is.
| Args | ||
|
|
||
| - form (forms.Form): Form to add to form set | ||
| """ |
There was a problem hiding this comment.
As per this comment, please remove this docstring addition.
| """ | ||
| Search pick must have at least one recommended page or link to be valid. | ||
| Check there is at least one non-deleted form with the required field. | ||
| """ |
There was a problem hiding this comment.
I personally think it makes sense to update this to docstring while changing, this will be fine.
| external_link_title = models.CharField( | ||
| _("Title"), max_length=32, blank=True, null=True | ||
| ) |
There was a problem hiding this comment.
Agree, this change still needs to be made.
|
|
||
| {% trans "Alternatively, use an external link for this Promotion" %} | ||
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_url only %} | ||
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_title only %} | ||
|
|
There was a problem hiding this comment.
| {% trans "Alternatively, use an external link for this Promotion" %} | |
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_url only %} | |
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_title only %} | |
| {% trans "Alternatively, use an external link for this Promotion" %} | |
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_url only %} | |
| {% include "wagtailadmin/shared/field.html" with field=form.external_link_title only %} |
For now, no need for the extra added whitespace.
| {% include "wagtailadmin/shared/field.html" with field=form.description only %} | ||
|
|
There was a problem hiding this comment.
no need to change this whitespace here.
| """ | ||
| Add search promotion with a Page and verify it appears in the | ||
| search promotion index page. | ||
| """ |
There was a problem hiding this comment.
Please remove all docstrings that are not part of new tests added.
This makes this PR have about 200 lines that are changed that have nothing to do with this PR.
| @@ -22,8 +22,15 @@ | |||
| </ul> | |||
| {% endif %} | |||
|
|
|||
| {% trans "Use an internal Page for this Promotion" %} | |||
There was a problem hiding this comment.
| {% trans "Use an internal Page for this Promotion" %} | |
| {% trans "Choose an internal page for this promotion" %} |
OR
| {% trans "Use an internal Page for this Promotion" %} | |
| {% trans "Select an internal page for this promotion" %} |
No need for these to be proper nouns, let's replace 'use' with 'choose' or 'select'
Thanks @TopDevPros - myself and another core team member have asked that all the extraneous docstrings. I understand that some of the existing code may not align 100% with pep8, although it does say...
For now, the goal of this PR is to add a single feature, not fix up the pep8 alignment of the entire contrib module. As such, please update this PR to be focused on that goal only. If it's useful, maybe you can raise a discussion about the Wagtail alignment with PEP8 and our non-use of some conventions (with code examples). From here, a plan can be put in place to either add nuance to our documentation or see how this alignment can be tackled. Hopefully the added feedback will help move this along, thank you. |
|
Thanks for the detailed suggestions @lb. We have removed all docstrings folowing your requirements and tried to implement all other suggestions. If we missed something, let us know. 🙂 |
There was a problem hiding this comment.
Thank you @TopDevPros
The only things left over are
- Changing the field
external_link_titletoexternal_link_text - Update a few validation messages to be a bit more consistent
- Re-add the existing docstrings in
wagtail/contrib/search_promotions/models.py- existing code can stay as is. - Fix up the verbose name on the URL field, it should be
'External link URL'. - Check the CI failures, looks like it's just formatting
Thank you, the PR is so much easier to review now.
A reminder to redo the migration once you make the model changes, so there is just one migration file with all the changes in it.
| raise forms.ValidationError(_("You must recommend a related page OR an external link.")) | ||
| else: | ||
| if external_link_url: | ||
| raise forms.ValidationError(_("Please only select a Page OR enter an external link.")) |
There was a problem hiding this comment.
| raise forms.ValidationError(_("Please only select a Page OR enter an external link.")) | |
| raise forms.ValidationError(_("Please only select a page OR enter an external link.")) |
| ) | ||
| sort_order = models.IntegerField(null=True, blank=True, editable=False) | ||
|
|
||
| external_link_url = models.URLField(_("URL"), blank=True) |
There was a problem hiding this comment.
Sorry but this still has not changed to the full label.
| external_link_title = models.CharField( | ||
| _("Title"), max_length=32, blank=True, null=True | ||
| ) |
There was a problem hiding this comment.
Can this please change to the suggested field name?
|
Wagtail is a great project, but we won't be contributing in the future. We usually love to handle tough challenges that no one has solved for years. We did that here. But this experience is not worth our time. We wish the wagtail team all the best. |
|
@TopDevPros that’s fair enough! It’s a bummer for me/us because your contributions I’m not sure exactly which aspect of our process you find isn’t working as well as you’d like (please feel free to expand if you have specific points of feedback for us, we’re always open to constructive criticism). At least from my perspective it’s somewhat common for there to be a fair bit of back and forth for first-time contributions of this size. Anyway, thanks for letting us know if you’re not wanting to work on this any further. It’ll make it easier for us to triage the pull request / ticket accordingly, and hopefully get someone else to pick this up in due time, seeing that the implementation seems really close. |
|
@thibaudcolas, Wagtail's UX is much better than any other CMS. We love it. The speed of feedback has been genuinely great. We appreciate the suggestions that enhance the code or user experience (e.g., require the external_link_title). Good developers disagree. There are some simple ways to resolve those disagreements quickly. Without resolution methods, disagreements quickly devolve to ego conflicts.
When recommendations were made to downgrade the quality of the software (e.g., remove code documentation, or replicate hard coded strings instead of using a shared constant), it was a surprise and disappointment. Wagtail is too good a project to advocate bad code practices. It is also concerning when such a small change to the code base seems to generate a flood of suggested changes that seem more like wanting their "fingerprints" on the code, and less like improvements. Even making the requested changes to downgrade the software just generated more fingerprints. It started to look like an endless time sink producing software we weren't proud of. We'd love to see this issue resolved. Obviously, the University of Chicago Library has been waiting a long time. Could you tell us who there requested this Search Promotions enhancement, so we can make sure they get what they need? Wagtail is the best in its class. We intend to continue using it and recommending it strongly to others. |
Unable to get any details on this error after trying many times. When I request details from CircleCI, I see: Recommended Fixes for pipenv run ruff check . Error Generating Recommendation An error occurred while generating a recommendation for this step output. Please try again later. Ideas? |
|
Hi, @TopDevPros, a tip: instead of clicking "Explain this error" which apparently uses CircleCI's "AI" that's unfortunately broken, the console output tends to give better explanations of what happened. In this case, the output is: So you need to remove the unused It hasn't been explicitly mentioned in the docs, but we recently adopted ruff to lint our Python code. The bits in the docs about running Also, thank you for your insights!
I agree that there is value to adding code docs and refactoring existing code. However, we tend to do it in separate PRs or at the very least in separate commits. When new code is introduced along with unrelated changes to existing code in the same commit, it can be distracting to the reviewer, especially when the PR is already quite big even without the unrelated changes. Just because some code is in the same file as the code that needs to be changed, it doesn't mean that the two are related in the context of the PR. As the author of a PR, it is often easier to focus and understand the changes, as you have been working on the code for some time before you submit the PR. However, for the reviewers, they have to understand the goal that you try to accomplish and why the changes are done in certain ways, think of possibly better alternatives or patterns that have already been established, etc. And, more importantly, I believe no one in the core team has complete understanding of the whole codebase to confidently review every PR that comes in; some of us know more in some parts of the codebase, and the others in other parts. As a result, the reviewers would likely also have to learn more about the codebase before reviewing. This is why keeping the scope of the PR focused on the goal is important, as it reduces the cognitive load on the reviewer (and speeds up the review process as a result). And on the topic of docstrings, we tend to only add docstrings where necessary. The most important docstrings are ones that explain more about the "why" of the code, not just what the code is/what it does. Just to give you an example, there was the following diff in the PR: class TestSearchPromotionsIndexView(WagtailTestUtils, TestCase):
+ """
+ Test search promotions index view.
+ """
def setUp(self):
+ """
+ Login before running test.
+ """
self.login()
While such docstrings are easy enough to be understood by the reviewer, it's hard to justify adding them when they do not have much value and are unrelated to the PR's goal. I wouldn't call adding them a good practice, and I'd argue not adding them would not "downgrade the quality of the software". Hence, I hope you understand why we made suggestions to remove them. Your contributions have been great and we really appreciate them, thus we were hoping the suggestions would give you better understanding of our PR scope expectations, and save you the time by keeping your PRs small should you make future contributions. Without the unrelated changes in the first place, the PR would be much smaller and would be easier to review (and eventually merge 😄) – And, some of those "unrelated changes" might still be relevant as a separate PR! Thank you again for your contributions and your feedback to us, we hope to do better in the future. |
|
@thibaudcolas, thank you for your clear answers to "Why". Certainly reducing cognitive load is critical. On that basis, keeping PRs small makes sense. You're also right that docstrings which don't add information or clarity are useless. This distinction wasn't obvious. There appeared to be a blanket ban on code documentation. Again, thank you for your thoughtful and informative response. |
There was a problem hiding this comment.
Thank you @TopDevPros - this is looking really solid, there were a few minor items of feedback left but I have just resolved these when reviewing.
I have also made a few final tweaks (list below) for UX improvements, I will squash, force push and if the CI is happy we are good to go.
- Use
help_textfor the help text for the different fields, this is more appropriate than just adding a translation above the fields - Added
help_textfor the description field, so that users do not get confused about this being for 'both' variations. - Updated all references to
Pageto be lowercasepagein validation/help text items. - Updated the listing link to be styled the same as the page links, add external link icon, remove
title(using title on aaelement has some complexities and should be avoided unless critical, the icon visually shows this as different and we can add a nice screen reader label for the icon easily). - Increased the link text char limit from 32 to 200, it's possible this limit is not required but I think it makes sense to have something a bit larger.
|
@TopDevPros I could not find a specific name for you / your team, so I have added you to the contributors list as |
Closes wagtail#10725 Built on original PR wagtail#4560
| "searchpicks_formset", | ||
| None, | ||
| None, | ||
| "Please only select a page OR enter an external link.", |
There was a problem hiding this comment.
Thank you for pulling these out from variable usage to 'hard-coding' in the unit tests.
While this can seem like duplication I just wanted to explain my reasoning for pushing for this approach instead of referencing a property on the class (as originally proposed in the PR). Others may disagree but I wanted to explain the 'why' so you see where I we were coming from.
- Accidental errors, if you used a property such as
MyModel.MY_CONSTANTand it is somehow malformed or wrong (e.g. an invalid string or not using translations nicely) and then your test says 'check error equalsMyModel.MY_CONSTANT'. This can hide legitimate errors, forcing the test to type out the desired value makes the test find these kinds of errors easier. - Unit tests that do not know about 'implementation'. While the boundary can be blurry, it's nice to be able to test against the desired output and ignore implementation. By testing against our ouput error, the code that generates/stores/contains this error can be changed without changing the unit test.
- Leaves room for future variable based translations, imagine we wanted to make this error based on some data (e.g. 'please select a page OR an external link, you have selected page 5'). Making this change requires our tests to look at partially re-implementing logic of this data usage OR just change to checking the output string anyway.
While it's not a hard rule, as in I am sure you will find tests that do 1:1 comparison with values for things like this, we tend to test against the output result and try to avoid our tests knowing too much about the implementation. This does lead to some duplication, but in my experience it means that the tests uncover more legitimate errors and also the tests only need to change when the output changes, not when the implementation driving the output changes.
I hope it's OK to give this feedback, I just wanted to make sure all your points of concern were addressed where possibile.
Fixes #10725
Added docstrings to all functions in forms.py, models.py, and tests.py.
Strongly suggest all tests in the future be careful not to assume the sort_order on newly created records if the sort_order was not explicitly set. Different databases can give different results (e.g., python3.10/django4.1/postgres12), at least in github's CI.
We're looking forward to hearing the core team's feedback on the UI. We've attached a few screenshots.
make lintfrom the Wagtail root.