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

Add ability to use form_ajax_refs fields as editable columns #2164

Closed
wants to merge 10 commits into from
Closed

Add ability to use form_ajax_refs fields as editable columns #2164

wants to merge 10 commits into from

Conversation

caffeinatedMike
Copy link
Contributor

@caffeinatedMike caffeinatedMike commented Oct 23, 2021

Adds support for inline-editing of form_ajax_refs fields from list.html views:

  • One-to-one relationship
    • Ajax search
    • Select2 field
    • Selections/adjustments persist/are saved to db
    • Editable field reflects the updated/selected value from the modal
  • One-to-many relationship
    • Ajax search
    • Select2 multiple field
    • Selections/adjustments persist/are saved to db
    • Editable field does not always reflect the updated (multiple) values once clicking save in the modal. Changes do reflect on refresh though.

🐛 Both single and multi-select inline editable fields are unable to have all their values deleted from the inline select2 field in the list.html views. This relates to #357 and should be addressed when that issue is addressed.

Closes #1627
Closes #2063
Closes #2157

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi would you mind checking my work here when you get the chance? Just want to make sure I'm seeing proper behavior in the areas I have marked as complete. And any insight on the ui glitches with the one-to-many would be appreciated :)

I've been using the following minimal, reproducible example:

from flask import Flask
from flask_admin import Admin
from flask_admin.contrib.sqla import ModelView
from flask_admin.contrib.sqla.ajax import QueryAjaxModelLoader
from flask_sqlalchemy import SQLAlchemy


app = Flask(__name__)
app.config.update({
    "SECRET_KEY": "NOTASECRET",
    "SQLALCHEMY_DATABASE_URI": "sqlite:///:memory:",
    "SQLALCHEMY_TRACK_MODIFICATIONS": False
})
db = SQLAlchemy(app)


class Structure(db.Model):
    __tablename__ = "structures"
    id = db.Column(db.Integer, primary_key=True)  # noqa
    name = db.Column(db.String(20), nullable=False)  # noqa
    power_unit_id = db.Column(db.Integer, db.ForeignKey("power_units.id"))  # noqa
    power_unit = db.relationship("PowerUnit", back_populates="structures")  # noqa

    def __str__(self):
        """Return a string representing the record."""
        return self.name


class PowerUnit(db.Model):
    __tablename__ = "power_units"
    id = db.Column(db.Integer, primary_key=True)  # noqa
    name = db.Column(db.String(20), nullable=False)  # noqa
    structures = db.relationship("Structure", back_populates="power_unit")  # noqa

    def __str__(self):
        """Return a string representing the record."""
        return self.name


class StructureView(ModelView):
    """Flask-Admin view for Structure model (structures table)"""

    column_list = ("name", "power_unit")
    form_columns = column_list
    column_editable_list = form_columns

    # test one-to-one
    form_ajax_refs = {
        "power_unit": QueryAjaxModelLoader(
            "power_unit", db.session, PowerUnit, fields=["name"]
        )
    }


class PowerUnitView(ModelView):
    """Flask-Admin view for PowerUnit model (power_units table)"""

    column_list = ("name", "structures")
    form_columns = column_list
    column_editable_list = form_columns

    # test one-to-many - currently works on a db-level, but UI-wise it's a bit odd after selecting values from the select2 and saving/closing modal.
    form_ajax_refs = {
        "structures": QueryAjaxModelLoader(
            "structures", db.session, Structure, fields=["name"]
        )
    }


# admin = Admin(app, url="/")
# admin = Admin(app, url="/", template_mode="bootstrap3")
admin = Admin(app, url="/", template_mode="bootstrap4")
admin.add_view(StructureView(Structure, db.session, name="Structures"))
admin.add_view(PowerUnitView(PowerUnit, db.session, name="Power Units"))


@app.before_first_request
def startup():
    db.create_all()
    num_items = 1000
    for i in range(num_items):
        db.session.add(Structure(name=f"a{i}b"))
        db.session.add(PowerUnit(name=f"a{i}b"))
        if i % 100 == 0 or i == num_items:
            # commit once every 100 records and once we finish iterating
            db.session.commit()


if __name__ == "__main__":
    app.run(debug=True)

@caffeinatedMike
Copy link
Contributor Author

@mrjoes As always, any thoughts from you are much appreciated as well :)

@michaelbukachi
Copy link
Contributor

@caffeinatedMike Testing as soon as I get home.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike

  • " Editable field does not always reflect the updated (multiple) values once clicking save in the modal. Changes do reflect on refresh though." ~ I think you have to update all x-editable columns on the current page as soon as save is done. So that the update values are shown.
  • Add:
    select2: {
                        dropdownAutoWidth: true, // <------ This
                        minimumInputLength: $el.attr("data-minimum-input-length"),
                        placeholder: "data-placeholder",

so the dropdown automatically adjusts the width according to the content.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Oct 23, 2021

@michaelbukachi thanks for having a look. Will definitely address the width issue prior to merge. Any idea how I can solve the first issue? Where do I even start? The issue only seems to happen sporadically when there are multiple values (so when select2 multiple/tag mode). I'm kind of at a loss currently as to how to properly handle the field population/refresh.

Another issue I noticed is when you add values through the inline edit and save, those values aren't available when clicking back into the field (so when the edit box is re-rendered) because data-json is not populated with the values that were selected from the edit box the first time around. If you refresh the page, those values will obviously show up in the edit box then, but it's unreasonable and eliminates the purpose of inline editing if we have to refresh the page on every inline edit "save".

@michaelbukachi
Copy link
Contributor

@michaelbukachi I'm playing around with your PR locally. Hopefully, I'll have a solution during the day.
"Another issue I noticed is when you add values through the inline edit and save, those values aren't available when clicking back into the field" - This is probably related to the first issue. The state is being lost on save. So we have to figure out a way to preserve it.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Oct 23, 2021

@michaelbukachi thanks for tinkering, please let me know how you fare.

I'm guessing when you say the state is not saved, you mean on the frontend (in data attributes of the html elements), correct? Because, just to be clear, the state does persist on the db-level (which can be seen by refreshing the page, it will show the updated tags).

@michaelbukachi
Copy link
Contributor

Yes. I'm referring to state on the frontend.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Oct 24, 2021

@michaelbukachi I think I might know what I'm missing. I think we might just need to add another setting to $el.editable({..

display: function(value) {
                        // override to display text instead of ids on list view
                        var html = [];
                        var data = $.fn.editableutils.itemsByValue(value, $el.data('source'), 'id') +

$.fn.editableutils.itemsByValue(value, $el.data('source'), 'value')
 
                        if(data.length) {
                            $.each(data, function(i, v) { html.push($.fn.editableutils.escape(v.text)); });
                            $(this).html(html.join(', '));
                        } else {
                            $(this).empty();
                        }
                    }

I won't be able to test this theory until tomorrow morning though. If you have the free-time and feel like it, feel free to do so :)

@michaelbukachi
Copy link
Contributor

@caffeinatedMike In which file are we adding this?

@caffeinatedMike
Copy link
Contributor Author

This would go in form.js. the above code might not be exactly what we need, but I believe some variation of this attribute of .editable will fix the table display.

@michaelbukachi
Copy link
Contributor

Alright. Let me test it.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Oct 26, 2021

@michaelbukachi I've fixed the issue with values that are added in the select2 box not persisting back to the table and vice-versa*, so no refresh is needed. One final hurdle is when removing an item in the select2 box it is not removed from the table until refresh. Getting close!

*Edit: Scratch that, select2 multiple only populates with values from the table after refresh. Need to figure out proper way to update <a value="?,?,?" data-value="?,?,?">.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike Noice! I was barely able to play around with the PR today. Glad to see you are making progress.

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi done! please review latest changes and let me know if everything works as expected :)

@michaelbukachi
Copy link
Contributor

@caffeinatedMike LG! However, when I select a relationship object which has already been selected, it doesn't deselect it from the the other selected row. See the GIF below:
vokoscreen-2021-10-28_05-36-57

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi Ahh, thanks for reminding me/pointing that out! I forget to include one of my local snippets that addressed that. Will update sometime today and re-ping you.

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi Sorry for falling off the map. It's been a long and tough week since my last comment. I finally have some time tomorrow to have a look at this.

@michaelbukachi
Copy link
Contributor

@michaelbukachi Sorry for falling off the map. It's been a long and tough week since my last comment. I finally have some time tomorrow to have a look at this.

No pressure 🙂

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi I'm just getting back to this. After looking at this, it seems it's a matter of determining the type of relationship (one-to-one or one-to-many). In the case of the Structures column at the /powerunit endpoint, it's a one-to-one (a structure can only be related to one powerunit). I'm struggling to figure out a way to figure out the relationship type ahead of time before the list view is rendered, otherwise it'll be as easy as adding a data-attribute to the column indicating which relationship type it is. From there I can use js to reflect the changes appropriately without refreshing the page.

I just can't see in the code where I can inject the additional data-attribute. Thoughts?

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Dec 3, 2021

@mrjoes You might be the better party to ask here since you built the internals. How could I go about passing on the type of relationship an Ajax field is dealing with? Ideally I'd like to add it as a data-attribute to the editable field, so we would ultimately need to pass this value to the xeditable widget, right? The question is how?

@michaelbukachi
Copy link
Contributor

@caffeinatedMike sorry for the late response. This slipped my mind so I forgot to take a look. I'll take a look at it during the day.

@michaelbukachi
Copy link
Contributor

@michaelbukachi I'm just getting back to this. After looking at this, it seems it's a matter of determining the type of relationship (one-to-one or one-to-many). In the case of the Structures column at the /powerunit endpoint, it's a one-to-one (a structure can only be related to one powerunit). I'm struggling to figure out a way to figure out the relationship type ahead of time before the list view is rendered, otherwise it'll be as easy as adding a data-attribute to the column indicating which relationship type it is. From there I can use js to reflect the changes appropriately without refreshing the page.

I just can't see in the code where I can inject the additional data-attribute. Thoughts?

Are fields aware of the data they hold? Or the models they are associated with? If we are able to determine the type as AjaxSelectField or AjaxSelectMultipleField maybe we can pull more information about the model. If that information does not exist, perhaps we can add new fields which extend the two mentioned which hold that information? 🤔. Let me try and prototype something.

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi Only while they're first being instantiated (example: within __call__(...) functions for input widgets). So, like all other data that is passed to those widgets via the kwargs, there definitely should be a way to pass on the relationship type if a field is one of the Ajax fields that contain the model.

After reviewing the codebase a little more, I believe the spot that is the most ideal for determining this is the create_editable_list_form function in flask_admin.model.form

@michaelbukachi
Copy link
Contributor

I see. Maybe we can introduce enums for the relationship types and pass them around?

@caffeinatedMike
Copy link
Contributor Author

My thoughts exactly, but haven't figured out where I can inject the data-attribute value other than on the widget level.

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi flask_admin.model.base > get_list_form function docstring shows a perfect example of how to add extra data-attributes to the widgets utilized in editable list fields, now we just need to figure out how to get the relationship type there. Could you have a look?

@michaelbukachi
Copy link
Contributor

@caffeinatedMike I've been playing with the widgets. I'm actually really close to figuring it out.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Dec 4, 2021

Awesome! Please let me know your findings when you do. This has been a thorn in my side for a bit. It's really the only thing stopping this PR from being merge-ready.

Starting Monday I'll be able to focus my attention on Flask-Admin full-time as I'm building the next iteration of a management portal I've created for my day job. So, if you figure it out before then it'll give a great starting point come Monday.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike We might not have to go through overriding get_list_form. Is the choice of using AjaxSelectField over AjaxSelectMultipleField or vice versa determined based on the relationship type? Or is it something the user has to specify 🤔 ?

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi It is. However, what we need is the opposite relationship. For example, in the code we're using to test:

form_ajax_refs = {
    "structures": QueryAjaxModelLoader(
        "structures", db.session, Structure, fields=["name"]
    )
}

What we need is to determine the direction.name of Structure.power_unit, which is MANYTOONE (many structures to one power_unit). Since the ModelView is for PowerUnit it isn't clear how to get there. PowerUnit.structures is accessible easy enough (which is ONETOMANY). We can't simply assume the relationships are absolute opposites since there are legitimate reasons for one end to enforce a ONETOONE as well.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike it should be possible to get the relationship model and infer relationship direction from there if we don't want to assume direct opposites.

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi That's what I've been trying to figure out: where to do this and how to ultimately pass it onto the widget kwargs.

@michaelbukachi
Copy link
Contributor

@michaelbukachi That's what I've been trying to figure out: where to do this and how to ultimately pass it onto the widget kwargs.
The first step is determining how is AjaxSelectField or AjaxSelectMultipleField selected.

@caffeinatedMike
Copy link
Contributor Author

flask_admin.contrib.sqla.form.AdminModelConverter functions _model_select_field and _convert_relation

@michaelbukachi
Copy link
Contributor

flask_admin.contrib.sqla.form.AdminModelConverter functions _model_select_field and _convert_relation

This is where we should determine the relationship and return the appropriate widget which has the relationship information.

@caffeinatedMike
Copy link
Contributor Author

Yes, that's why I was pointing it out... I'm currently tied up with a few other tasks at the moment, but putting it here as a note.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike so checking _convert_relation, as long a relationship is ONETOMANY, MANYTOMANY and uselist is set then AjaxSelectMultipleField is used. Anything else MANYTONE or if uselist is false (meaning ONETOONE) then AjaxSelectField is used. So just checking the type of field being used is more than enough.

@caffeinatedMike
Copy link
Contributor Author

caffeinatedMike commented Dec 24, 2021

@michaelbukachi I'm just getting back to this now. The logic you provided doesn't really help when looking at the example code we've been using to test. In your gif of the bug each PowerUnit can have many Structures, but a Structure can only belong to a single PowerUnit at a time. So, just because the inline field is a SelectMultiple that doesn't mean we can assume that values should remain. The values should only remain in MANYTOMANY (our situation deals with a ONETOMANY). Net net, we need a different way to determine this because using just the field type of SelectMultiple as an indicator would mean having a 50% chance of getting it wrong.

@michaelbukachi
Copy link
Contributor

@caffeinatedMike what needs to be displayed for MANYTOMANY as opposed to ONETOMANY. Maybe I'm interpretating this in the wrong way 🤔

@caffeinatedMike
Copy link
Contributor Author

@michaelbukachi nothing needs to be done for MANYTOMANY. For ONETOMANY we need some sort of data-attribute to let us know to iterate over all other entries present on the current page in order to remove the value from those. In your gif you point out how the element is not removed from the other field until refresh. Thats the situation where we need a signifier to tell us to do this via JS to avoid needing a page refresh.

@mccarthysean
Copy link
Contributor

FYI, I didn't realize you had done this already. I added the same functionality in the following pull request:

#2256

@caffeinatedMike caffeinatedMike closed this by deleting the head repository Sep 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants