Skip to content

Implement metadata only parsing for Items #8

Closed
campb303 opened this issue Aug 6, 2020 · 9 comments · Fixed by #113
Closed

Implement metadata only parsing for Items #8

campb303 opened this issue Aug 6, 2020 · 9 comments · Fixed by #113
Assignees
Labels
enhancement Request for a change to existing functionality high-priority Needs immediate extra focus

Comments

@campb303
Copy link
Collaborator

campb303 commented Aug 6, 2020

Currently, when an Item is loaded, all of its contents including its body are parsed. As more features such as attachment parsing are added, this could slow down the loading of the ItemTable in the UI by:

  • Parsing unnecessary information like body sections and attachment contents
  • Transferring whole items with body and attachments when only metadata is needed

This could be addressed by implementing a function that only loads information needed to display in the ItemTable and waiting to load the rest of the item until it is requested in the ItemView.

@campb303 campb303 added enhancement Request for a change to existing functionality api tooling Related to tools and utilities for the management of the project labels Aug 7, 2020
@campb303 campb303 added this to the v1 milestone Sep 14, 2020
@campb303
Copy link
Collaborator Author

With multiple queue loading allowed now, this issue is more important as load times now exceed 10 seconds.

@campb303
Copy link
Collaborator Author

Also, Sundeep wants a Jester loader.

@campb303 campb303 added high-priority Needs immediate extra focus and removed tooling Related to tools and utilities for the management of the project labels Nov 25, 2020
@campb303
Copy link
Collaborator Author

This is waiting for a code review.

@benne238
Copy link
Collaborator

Header Only Parsing

In pull request #113, this issue was addressed by modifying the arguments for Item.
The arguments for Item are now 'queue', 'item_number', and 'get_full_item'.

Argument Description
queue The queue the item is located in
number The item number of the item
wholeItem A boolean representing if the full item should be returned

True: Return full item
False: Return only the headers for the item

This was done by modifying which attributes of the Item class in ECNQueue are defined based on the value of wholeItem:

def __init__(self, queue: str, number: int, wholeItem: bool) -> None:

      # Code was intentionally omitted here for relevancy

      if wholeItem: self.content = self.__parseSections()

The entirety of the item content, which is stored in self.content is only returned if the wholeItem variable is True. The wholeItem variable is only True when a specific item is requested, such as Me 11, otherwise it is False, such as when an entire queue (or multiple queues) are selected, such as Me, Ce, and Cgt without a specific item reference.

@campb303
Copy link
Collaborator Author

This functionality has been modified and extended.

ECNQueue Module

Both Items and Queues has an argument in their constructor called headersOnly that indicates whether or not to parse the contents of an item. For Items this will default to False, loading the content, for Queues this will default to True, not loading the content for a Queue's items.

Examples:

import ECNQueue

# Create an Item with content
item = Item("ce", 100)
# Create an Item without content
item = Item("ce", 100, headersOnly=True)

# Create a Queue with content
queue = Queue("ce", headersOnly=False)
# Create a Queue without content
queue = Queue("ce")

API

In the API, the Item and Queue constructors are used with the same defaults and customization options. A client can control this option using a URL's query string.

Examples:

# Request an Item with content
curl 'localhsot:5000/ce/100'
# Request an Item without content
curl 'localhsot:5000/ce/100?headersOnly=True'

This is parsed in the API using the args property of Flask's request object. This works with Flask RESTful's resource because a Flask RESTful Resource class extends the Flask request class. Because the query string is passed as a string, we can use a string comparison to parse a boolean value and pass that value to an Item and Queue constructor.

Examples:

from flask import Flask, request
from flask_restful import Api, Resource
import ECNQueue

# Create Flask App
app = Flask(__name__)

# Create API Interface
api = Api(app)

# Create a Flask RESTful Resource for dealing with Items
class Item(Resource):
    # Create a method to handle HTTP GET requests
    def get(self, queue: str, number: int) -> str:
        # Parse a boolean value from the query string using Flask's request object
        headersOnly = True if request.args.get("headersOnly") == "True" else False
        return ECNQueue.Item(queue, number, headersOnly=headersOnly).toJson()

# Add the Item resource to routing
api.add_resource(Item, "/api/<string:queue>/<int:number>")

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

@campb303
Copy link
Collaborator Author

campb303 commented Dec 1, 2020

Next steps are to merge this into staging and update the frontend' logic. This should happen after #123

@campb303 campb303 removed the high-priority Needs immediate extra focus label Dec 7, 2020
@benne238 benne238 self-assigned this Feb 1, 2021
@benne238 benne238 linked a pull request Feb 1, 2021 that will close this issue
@campb303 campb303 added the high-priority Needs immediate extra focus label Feb 5, 2021
@campb303
Copy link
Collaborator Author

campb303 commented Feb 5, 2021

This does not depends on #123 and can be merged into the UI now.

@campb303 campb303 modified the milestones: v1-proof-of-concept, v2-production-ready-read-only Feb 5, 2021
@campb303
Copy link
Collaborator Author

campb303 commented Mar 2, 2021

After introducing the ItemProvider component to consolidate item management, a regression has been introduced causing recursive variable setting. The particular issue occurs when ItemView's useEffect attempts to load the item body and calls setActiveItem. This will trigger a re-redner of AppView which in turn calls setActiveItem overwriting activeItem with the empty Item from the ItemTable rather than the complete Item from the API. (See log below.)

The routing logic needs to be memoized to avoid this issue.

log.js:24 [HMR] Waiting for update signal from WDS...
index.js:1 Warning: Cannot update a component (`ItemProvider`) while rendering a different component (`Context.Consumer`). To locate the bad setState() call inside `Context.Consumer`, follow the stack trace as described in https://fb.me/setstate-in-render
    in Route (at AppView.js:128)
    in div (created by Styled(MuiBox))
    in Styled(MuiBox) (at AppView.js:126)
    in div (created by ForwardRef(Paper))
    in ForwardRef(Paper) (created by WithStyles(ForwardRef(Paper)))
    in WithStyles(ForwardRef(Paper)) (created by Styled(MuiBox))
    in Styled(MuiBox) (at AppView.js:113)
    in AppView (at App.js:21)
    in Route (at PrivateRoute.js:10)
    in PrivateRoute (at App.js:20)
    in Switch (at App.js:16)
    in ThemeProvider (at App.js:15)
    in App (at src/index.js:24)
    in Router (created by BrowserRouter)
    in BrowserRouter (at src/index.js:23)
    in ItemProvider (at src/index.js:22)
    in AuthProvider (at src/index.js:21)
    in CookiesProvider (at src/index.js:20)
    in StrictMode (at src/index.js:18)
console.<computed> @ index.js:1
overrideMethod @ react_devtools_backend.js:2430
printWarning @ react-dom.development.js:88
error @ react-dom.development.js:60
warnAboutRenderPhaseUpdatesInDEV @ react-dom.development.js:23241
scheduleUpdateOnFiber @ react-dom.development.js:21165
dispatchAction @ react-dom.development.js:15660
render @ AppView.js:141.                               // Bad Line
(anonymous) @ Route.js:55
updateContextConsumer @ react-dom.development.js:18304
beginWork @ react-dom.development.js:18661
beginWork$1 @ react-dom.development.js:23179
performUnitOfWork @ react-dom.development.js:22154
workLoopSync @ react-dom.development.js:22130
performSyncWorkOnRoot @ react-dom.development.js:21756
(anonymous) @ react-dom.development.js:11089
unstable_runWithPriority @ scheduler.development.js:653
runWithPriority$1 @ react-dom.development.js:11039
flushSyncCallbackQueueImpl @ react-dom.development.js:11084
flushSyncCallbackQueue @ react-dom.development.js:11072
discreteUpdates$1 @ react-dom.development.js:21893
discreteUpdates @ react-dom.development.js:806
dispatchDiscreteEvent @ react-dom.development.js:4168

@campb303
Copy link
Collaborator Author

campb303 commented Mar 11, 2021

The above issue with successive state has been fixed by no longer setting state in the ItemView Route. Instead the item lookup has been broken into two parts.

First the existing ItemTable onClick behavior has not been modified and will append /queue/number to the URL when a the corresponding row has been clicked. e.g. clicking on the row for bidc1 will append /bidc/1 to the URL. This is accessible in the ItemView Route via react-router's match object and passed to the ItemViewHeaderBar for a title.

That same data is also passed to the ItemView component where it is used to query the API for the full item and set state.

Example:
Header Loading

There are still some unsynced UI elements that need updated for this to be an elegant solution but it is functional.

Moving forward, ItemTable's performance will need to be addressed as it is the bottleneck due to it re-rendering on every interaction.

Sign in to join this conversation on GitHub.
Labels
enhancement Request for a change to existing functionality high-priority Needs immediate extra focus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants