Skip to content
Snippets Groups Projects

Draft: Functionality to add shopping items to basket from IDA.

Open Draft: Functionality to add shopping items to basket from IDA.
add_to_basket into master
3 unresolved threads
Open Hugh Dickinson requested to merge add_to_basket into master
3 unresolved threads

WIP: Users can now add one or more shopping items to their basket. No validation has been implemented. this should probably be delegated to archive-specific plugins.

Edited by Hugh Dickinson

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
shopping_client/shopping_client.py
108 """
109
110 url = urllib.parse.urljoin(self.host, shopping_client.endpoint)
111
112 if self.username is None:
113 # Retrieve username
114 response = requests.get(url, headers=self._request_header())
115 if response.ok:
116 self.username = json.loads(response.content)["results"][0]["user_name"]
117 else:
118 warn(
119 f"Unable to retrieve username from {self.host}; is your key valid?"
120 )
121 return False, response
122
123 # PATCH not working properly need to add existing basket items
  • John Swinbank
    John Swinbank @swinbank
    Owner

    Not sure I understand this comment.

  • Hugh Dickinson
    Hugh Dickinson @hughdickinson
    Author Developer

    Basically, I think it should be possible to just add to the list of shopping items using patch, but in fact one has to replace the original with a complete updated version. This is probably expected behaviour, but I wasn't expecting it. Maybe because the list of shopping items is a single field in the user profile model.

  • John Swinbank
    John Swinbank @swinbank
    Owner

    Ah! You're thinking of an HTTP PATCH? I was reading this as something like a list.patch method in Python, and there isn't one, and I was confused. Ok, that makes sense.

  • Please register or sign in to reply
  • John Swinbank
    John Swinbank @swinbank started a thread on the diff
    • shopping_client/shopping_client.py
    87 91
    88 92 return self.basket
    89 93
    94 def add_to_basket(self, items: list):
    95 """Add items to user's shopping basket.
    96
    97 Parameters
    98 ----------
    99 items : list
    100 List of objects, each containing the data for a single
    101 shopping item. Objects should be JSON-serializable. Examples
    102 include Python `dict`s or `list`s.
    103
    104 Returns
    105 -------
    106 bool
  • John Swinbank
    John Swinbank @swinbank started a thread on the diff
    • shopping_client/shopping_client.py
    87 91
    88 92 return self.basket
    89 93
    94 def add_to_basket(self, items: list):
    95 """Add items to user's shopping basket.
    • John Swinbank
      John Swinbank @swinbank
      Owner

      I'm wondering about the semantics of this.

      As I read the code (I confess, I've not tested it directly), the user will normally get a list (or a DataFrame) by calling get_basket(). Isn't the natural thing for them to do to simply manipulate that list — adding, removing, altering items — and have those changes synced back to ESAP (either automatically, or on demand)?

      Maybe easier to put that in pseudocode. The current workflow would look something like this:

      basket = shopping_client.get_basket()
new_basket_entry = "A thing what I made up"
shopping_client.add_to_basket([new_basket_entry])

      I guess what seems weird in the above API is that I'm manipulating the contents of the shopping basket by calling methods on some other objects.

      I'm imagining an API that looks more like:

      basket = shopping_client.get_basket()
basket.append("A thing what I made up")
shopping_client.sync()
    • Hugh Dickinson
      Hugh Dickinson @hughdickinson
      Author Developer

      This is possible but it would require some refactoring. Basically the basket would have to be implemented as its own class that provides an append method. Not sure how to expose the underlying data container DF/list either. Some of the plugins may rely on certain behaviour. Tricky.

    • John Swinbank
      John Swinbank @swinbank
      Owner

      Is the basket not a list? (Leaving aside DataFrames for the moment for simplicity). Maybe not; I should look in my Copious Free Time.

    • Hugh Dickinson
      Hugh Dickinson @hughdickinson
      Author Developer

      If the user doesn't set the convert_to_pandas argument True in get_basket() then the internal representation is just a list, yes. Otherwise, it's a pandas data frame. Notwithstanding, I think what you're talking about would involve making a Basket class with a list-like interface, or which at least implements append(). Plus I'm assuming you'd either want append to immediately update the basket entry in the database or to have a save() or commit() method too (and something like a to_pandas() method for users who do want a pandas data frame to work with). It's not difficult to do that, but it is a major API change and I'm not sure if any of the plugins developed so far rely on the convert_to_pandas functionality. It might also be worth asking the various archive plugins to provide a make_basket_entry() method to avoid the hacking that Nico mentions in esap-general#68 (comment 23686)

      Edited by John Swinbank
    • John Swinbank
      John Swinbank @swinbank
      Owner

      Hi Hugh — sorry for leaving this idle for so long.

      Basically, yes, I think your summary of my requests is about right. I'm worried that the API currently in this MR is confusing to the naive user as to what's going on. The fact that I can do

      >>> len(shoppingClient.get_basket())
2
>>> shoppingClient.get_basket().append("Some data")
>>> len(shoppingClient.get_basket())
3

      without actually adding anything to the shopping basket seems weird; worse is that if I then try to add something using the “correct” method it explodes:

      >>> shoppingClient.add_to_basket("some data")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jds/Projects/ASTRON/src/git.astron.nl/astron-sdc/esap-userprofile-python-client/shopping_client/shopping_client.py", line 125, in add_to_basket
    "shopping_cart": [
  File "/Users/jds/Projects/ASTRON/src/git.astron.nl/astron-sdc/esap-userprofile-python-client/shopping_client/shopping_client.py", line 126, in <listcomp>
    json.loads(item["item_data"]) for item in self.get_basket()
TypeError: string indices must be integers

      My fairly off-the-cuff suggestion is that ShoppingClient should be a MutableSequence which stores the “raw” basket as a list of JSON strings internally. A get_as_pandas method could return a copy of the data as a dataframe. I can think more about this and write down a concrete proposal if that would be helpful (but not today...).

      I think it's better to break the API now, before we have any widespread deployments; changes will only get more painful with time.

      Thoughts welcome! Please push back if you think I'm crazy... :-)

    • Hugh Dickinson
      Hugh Dickinson @hughdickinson
      Author Developer

      Hi John,

      I don't think it will be difficult to implement what you want but a concrete proposal would indeed be useful. No need to have it immediately though :-)

      Cheers,

      Hugh

    • Please register or sign in to reply
    Please register or sign in to reply