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

Ordering listing elements #677

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Ordering listing elements #677

wants to merge 11 commits into from

Conversation

jum-s
Copy link
Contributor

@jum-s jum-s commented Apr 17, 2023

Allowing users to reorder elements in a list.
Adds a ordinal field to elements documents

most important commit: 40a8d56

  • Storing reorderable position have lots of possible implementations, especially with noSql databases. But until we experience performance problems, we could stick to this proposed implementation : position integer paradigm (although alphabetical rank looks fun to explore).
  • ordinal naming improves grepability as position is already used by user.position = [lat, lon]
  • Starts first ordinal from 0 to avoid some off-by-one mess, but at the cost of returning falsy value (aka 0)
  • Script to assign ordinals may be deleted after production db update

@maxlath maxlath force-pushed the propagate-listing-elements-redirections branch from ca9add4 to 2967604 Compare August 29, 2023 10:10
@inventaire inventaire deleted a comment from maxlath Feb 15, 2024
@jum-s jum-s closed this Feb 15, 2024
@jum-s jum-s reopened this Feb 15, 2024
@jum-s jum-s force-pushed the ordering-listing-elements branch from fdc5b68 to 8e5ba73 Compare March 20, 2024 15:30
@jum-s jum-s changed the base branch from propagate-listing-elements-redirections to main March 20, 2024 15:31
@jum-s jum-s changed the title [WIP] Ordering listing elements Ordering listing elements Mar 20, 2024
@jum-s jum-s requested a review from maxlath March 20, 2024 15:51
@jum-s jum-s force-pushed the ordering-listing-elements branch from 8e5ba73 to e46af93 Compare March 25, 2024 10:04
@jum-s jum-s force-pushed the ordering-listing-elements branch from e46af93 to bf374ce Compare April 11, 2024 08:34
@jum-s
Copy link
Contributor Author

jum-s commented Apr 11, 2024

In order to test the script to fill elements without ordinal, one may:

  • delete lists and elements couch databases,
  • replace assert_.number(element.ordinal) by delete element.ordinal
  • skip test xit('should not add twice an element already in listing' (otherwise, it will create 2 elements with same list and uri which should be forbidden)
  • run tests/api/listings/
  • check that no element have an ordinal key
  • run scripts/db_actions/assign_elements_ordinal_to_all_listings.ts
  • check that all elements have an ordinal key

jum-s added 7 commits May 18, 2024 09:08
see [possible implementations](https://softwareengineering.stackexchange.com/questions/195308/storing-a-re-orderable-list-in-a-database)
Choice was made to have the simpliest implementation for the moment: position integer.
with a safer way to assign ordinal, not relying on loop (which is based on
array length) but on existing ordinal, to better handle legacy element
without ordinal
- with a temporary script to create ordinals on existing elements
- allow to update ordinals, by accepting paginated elements subset for performance and client friendlyness
- should test that removing element then reordering is ok
only element's byListings view existed before. This commit splits up server code between byListing and byListings queries.

The code shoud be more efficient as it does not sort by ordinal after couch view, leaving the sorting to couchdb

This couch views split allowed some neat refactoring on the way
@jum-s jum-s force-pushed the ordering-listing-elements branch from 6119af7 to 4cfbe0a Compare May 18, 2024 07:14
jum-s added 4 commits May 18, 2024 09:26
allows only to update `comment` attribute
the temporary function 'updateElementDoc' is not reordering oridnals yet, waiting for next commit to implement lexicographic ordering
easing client requirements by assigning a lexicographical order only in Element model (as close as possible to the db)

this may need some debouncing system to not conflict update couchdb.

but i cant see any implementation that would be debouncing-free
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants