Add a way to reach specific training modules and slides via ID numbers #5502

gabina · 2023-10-08T03:51:24Z

What this PR does

The purpose of this PR is to add:

A way to reach a training module by their id.
A way to reach an individual slide by their id.

In order to do that, two new routes were added (find_training_module and find_training_slide), as long as their corresponding controllers and specs.

The logic behind the controllers is to redirect the user to the URL for the given training module (/training/library-slug/module-slug) or to the individual slide (/training/library-slug/module-slug/slide-slug).

Closes #4940

Screenshots

After this PR, users are redirected to the training module or slide site.

Screencast.from.08-10-23.23.50.00.webm

Open questions and concerns

… to be able to reach a training module by their id

… be able to reach individual slides by id

gabina · 2023-10-08T20:10:48Z

spec/factories/training_slides.rb

 FactoryBot.define do
  factory :training_slide do
+    id { 456875}
+    title { 'How to create a slide' }
+    slug { 'how-to-create-a-slide' }
  end
 end


Added this because I want to test the behavior for an orphan slide (one that doesn't belong to any module).

…rom module slug, and 2) a module from slide slug

gabina · 2023-10-09T20:01:52Z

Thanks for approving the workflow. It looks like the new TrainingController specs I added passed. However, CI failed. By taking a look at the failed examples I think it's not related to my changes.

Failed examples:

rspec ./spec/features/article_finder_spec.rb:17 # article finder performs searches and returns results
rspec ./spec/lib/lift_wing_api_spec.rb:10 # LiftWingApi returns the basically the same data as OresApi
rspec ./spec/lib/lift_wing_api_spec.rb:20 # LiftWingApi handles TextDeleted revs similarly
rspec ./spec/lib/ores_api_spec.rb:29 # OresApi#get_revision_data fetches json from ores.wikimedia.org for wikipedia
rspec ./spec/lib/ores_api_spec.rb:37 # OresApi#get_revision_data fetches json from ores.wikimedia.org for wikidata
rspec ./spec/lib/ores_api_spec.rb:46 # OresApi#get_revision_data handles many revisions per request for wikipedia
rspec ./spec/lib/ores_api_spec.rb:53 # OresApi#get_revision_data handles many revisions per request for wikidata
rspec ./spec/lib/replica_spec.rb:88 # Replica API requests returns a list of existing articles

Edit: ragesoss told me I can ignore those ^^

ragesoss · 2023-10-09T22:50:25Z

app/controllers/training_controller.rb

+    training_module = find_module_from_slide_slug(training_slide.slug)
+    raise ActionController::RoutingError, 'module not found' unless training_module
+    training_library = find_library_from_module_slug(training_module.slug)
+    raise ActionController::RoutingError, 'library not found' unless training_library


It's possible to have a module that is not in any library, and it would render if you visited a path that had any valid training library slug. (You can view a module from one library even if you substitute the slug from another library.)

It would be good to find the library as you do here, but if no library is found, it would be better to use TrainingLibrary.first as a fallback instead of throwing a 404.

hmm interesting. That behavior is not so intuitive I guess.

I changed the logic for the controllers and updated specs on fbb894f

Note that the new definitions may result in failure if no library exists. In such a scenario, calling TrainingLibrary.first would return nil, causing an error when attempting to access the slug.
Not sure if a world with no libraries makes sense, or it is not worth considering this case and we can suppose TrainingLibrary.first is always well defined.
It looks like modules and slides render even using a non-existing library slug, so we could use a literal string
'mylibrary' as default library slug value if no libraries at all.
What's your opinion here?

ragesoss · 2023-10-09T22:56:33Z

app/helpers/training_helper.rb

+  # Given a module slug, it returns the first library that has a
+  # category including that module slug. It returns nil if no such
+  # library is found.
+  def find_library_from_module_slug(module_slug)


I think both of these helper methods would make more sense as instance methods of TrainingSlide and TrainingModule respectively.

Yep, agreed. I made all your suggested changes on 54f8848

ragesoss · 2023-10-09T23:00:00Z

app/helpers/training_helper.rb

+    end
+  end
+
+  # Given a slide slug, it returns the first module including it.


As a TrainingSlide instance method, you could implement this as

TrainingModule.all.detect { |tm| tm.slide_slugs.include? slug }

Much more idiomatic, I like it!

ragesoss · 2023-10-09T23:07:35Z

This looks great overall.

ragesoss · 2023-10-09T23:13:09Z

app/helpers/training_helper.rb

+  # category including that module slug. It returns nil if no such
+  # library is found.
+  def find_library_from_module_slug(module_slug)
+    TrainingLibrary.all.find_each do |library|


For this method, I would suggest implementing TrainingLibrary#training_module_slugs (for example:

def training_module_slugs categories.map do |cat| cat['modules'].map { |mod| mod['slug'] } end.flatten end

)

and then using that to implement TrainingModule#library:

TrainingLibrary.all.detect { |tl| tl.training_module_slugs.include? slug }

…e TrainingLibrary.first as a default library instead.

…brary from slug as instance methods instead

…ion in controllers

gabina · 2023-10-10T16:18:28Z

app/models/training_module.rb

+  # Returns a specific training library for the module,
+  # or a default library if it is not found.
+  def find_or_default_library
+    find_library_by_slug || TrainingLibrary.first
+  end


I created this method only to avoid code duplication in controllers.

Alternatively, I guess I could define just something like this:

# Returns the first library that has a category including the module slug, # or a default library if it is not found. def find_library_by_slug_or_default TrainingLibrary.all.detect(-> { TrainingLibrary.first }) { |tl| tl.training_module_slugs.include? slug } end

ragesoss · 2023-10-10T18:52:57Z

Nice work!

Add a new find_training_module route and its corresponding controller…

dcfd285

… to be able to reach a training module by their id

gabina marked this pull request as draft October 8, 2023 03:53

gabina added 5 commits October 8, 2023 01:39

Small stylistic change in controller

75129a5

Minor change in controller and addition of new spec for it

13705ae

Add new find_training_slide route and its corresponding controller to…

edc0873

… be able to reach individual slides by id

Add spec to test new find_slide method in training controller

a626846

Upload missing file from previous commit

ef2c7c5

gabina commented Oct 8, 2023

View reviewed changes

gabina added 2 commits October 8, 2023 23:17

Add a TrainingHelper class to include functions to find 1) a libary f…

5918b20

…rom module slug, and 2) a module from slide slug

Minor stylistic change

a1d2a54

gabina marked this pull request as ready for review October 9, 2023 02:52

gabina mentioned this pull request Oct 9, 2023

Add a way to reach specific training modules and slides via ID numbers #4940

Closed

gabina changed the title ~~Add a way to reach specific training modules and slides via ID numbers [WIP]~~ Add a way to reach specific training modules and slides via ID numbers Oct 9, 2023

Apply RuboCop on the changed files

dfcfd8c

ragesoss reviewed Oct 9, 2023

View reviewed changes

gabina added 3 commits October 10, 2023 00:21

Stop raising RoutingError for modules that are not in any library. Us…

fbb894f

…e TrainingLibrary.first as a default library instead.

Remove TrainingHelper module and define methods to find module and li…

54f8848

…brary from slug as instance methods instead

Define find_or_default_library instance method to avoid code duplicat…

3c2fadc

…ion in controllers

gabina commented Oct 10, 2023

View reviewed changes

gabina requested a review from ragesoss October 10, 2023 16:20

ragesoss merged commit d1a587b into WikiEducationFoundation:master Oct 10, 2023
1 check failed

gabina deleted the AddFunctionalityToReachSpecificTrainingModulesAndSlidesViaIDNumbers branch October 10, 2023 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to reach specific training modules and slides via ID numbers #5502

Add a way to reach specific training modules and slides via ID numbers #5502

gabina commented Oct 8, 2023 •

edited

Loading

gabina Oct 8, 2023

gabina commented Oct 9, 2023 •

edited

Loading

ragesoss Oct 9, 2023

gabina Oct 10, 2023

ragesoss Oct 9, 2023

gabina Oct 10, 2023 •

edited

Loading

ragesoss Oct 9, 2023

gabina Oct 10, 2023

ragesoss commented Oct 9, 2023

ragesoss Oct 9, 2023

gabina Oct 10, 2023 •

edited

Loading

ragesoss commented Oct 10, 2023

Add a way to reach specific training modules and slides via ID numbers #5502

Add a way to reach specific training modules and slides via ID numbers #5502

Conversation

gabina commented Oct 8, 2023 • edited Loading

What this PR does

Screenshots

Open questions and concerns

gabina Oct 8, 2023

Choose a reason for hiding this comment

gabina commented Oct 9, 2023 • edited Loading

ragesoss Oct 9, 2023

Choose a reason for hiding this comment

gabina Oct 10, 2023

Choose a reason for hiding this comment

ragesoss Oct 9, 2023

Choose a reason for hiding this comment

gabina Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

ragesoss Oct 9, 2023

Choose a reason for hiding this comment

gabina Oct 10, 2023

Choose a reason for hiding this comment

ragesoss commented Oct 9, 2023

ragesoss Oct 9, 2023

Choose a reason for hiding this comment

gabina Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

ragesoss commented Oct 10, 2023

gabina commented Oct 8, 2023 •

edited

Loading

gabina commented Oct 9, 2023 •

edited

Loading

gabina Oct 10, 2023 •

edited

Loading

gabina Oct 10, 2023 •

edited

Loading